Replace civet with hyper as http server
#1378
Conversation
jtgeibel
force-pushed
the
conduit-hyper
branch
2 times, most recently
from
ca76ffc to
7b75849
Compare
|
Status update: I've rewritten There are a few notes in the README file but none of them should affect us here. For instance, we don't care about the IP address of the request because it will always be the nginx reverse proxy so I haven't bothered to add the infrastructure to pull the connection IP address out of hyper. |
|
We very much do care about the IP address of the request. It is used for logging as well as for blacklisting. |
|
Oh I see what you're saying. We don't care about the remote addr, we're just pulling it from the x-forwarded-for header |
1394: Update tests to use hyper 0.12 r=sgrif This PR updates the HTTP recording test infrastructure to use hyper 0.12. The goal is to update hyper in the tests now to minimize any version changes when eventually moving production over to hyper (see companion PR #1378). While we don't *have* to use the same version of hyper in production and our tests, having different versions would regress compile times. Ignoring the crate additions/removals (as production code obviously don't depend on them), the only overlap I see with production is updating `libc` from 0.2.33 to 0.2.40. As far as I can tell, the following changes do not affect production, even transitively (and if they do, are minimal): * `net2` - 0.2.31 to 0.2.32 * `bytes` - 0.4.5 to 0.4.7 * `futures` - 0.1.17 to 0.1.21 * `tokio-core` - 0.1.12 to 0.1.17 * `tokio-io` - 0.1.3 to 0.1.6 It was necessary to lowercase the request header names in the recordings under `http-data` because the hyper client sending the request no longer uses title-case. [source]: https://twitter.com/seanmonstar/status/992156132411035648
1394: Update tests to use hyper 0.12 r=jtgeibel This PR updates the HTTP recording test infrastructure to use hyper 0.12. The goal is to update hyper in the tests now to minimize any version changes when eventually moving production over to hyper (see companion PR #1378). While we don't *have* to use the same version of hyper in production and our tests, having different versions would regress compile times. Ignoring the crate additions/removals (as production code obviously don't depend on them), the only overlap I see with production is updating `libc` from 0.2.33 to 0.2.42. As far as I can tell, the following changes do not affect production, even transitively (and if they do, are minimal): * `net2` - 0.2.31 to 0.2.32 * `bytes` - 0.4.5 to 0.4.8 * `futures` - 0.1.17 to 0.1.21 * `tokio-core` - 0.1.12 to 0.1.17 * `tokio-io` - 0.1.3 to 0.1.6 It was necessary to lowercase the request header names in the recordings under `http-data` because the hyper client sending the request no longer uses title-case.
|
Okay, I've published the I do have a PR against the crate that catches panics and converts them to a status 500 response, however the default hyper behavior of simply closing the connection on a panic matches our current Similar to the EnsureWellFormed500 middleware, This PR should now be ready for review. |
`cargo update -p conduit-hyper --aggressive`
jtgeibel
changed the title
civet with hyper as http servercivet with hyper as http server
|
LGTM, I'm going to merge and deploy and watch. bors: r+ |
1378: Replace `civet` with `hyper` as http server r=carols10cents a=jtgeibel This is a work in progress implementation switching from `civet` to `hyper` as the base http server. This is based on `conduit-hyper` which I've updated to `hyper 0.11`. That source code is [here](https://github.com/jtgeibel/conduit-hyper/blob/master/src/lib.rs). A huge shout-out to @sfackler and @drbawb as this is forked from their repos. I'm curios if either of you are still using this code and if you have any feedback. I plan to eventually publish this crate on crates.io, so please let me know if you would like to be added/removed as an author or if you have any objections. There are several things I wish to investigate further, including: * I'm not yet sure if hyper handles panics. I see no results searching for `catch_unwind`, so I'm assuming panics need to be handled in this layer. * Add some tests * request.remote_addr() is deprecated, also see: hyperium/hyper#1410 (comment) Co-authored-by: Justin Geibel <[email protected]>
Build succeeded |
1476: Revert "Merge #1378" r=carols10cents a=jtgeibel This reverts commit ba6c689, reversing changes made to 9b4a92c. This reverts the changes introduced in PR #1378. The deploy has already been rolled back and this will get master back into a clean state so that deploys can be made if other issues arise before this bug can be resolved. The code reverted by this PR was causing `cargo publish` to fail with a 404 error. The issue appears to be triggered because `cargo publish` includes an additional `/` at the beginning of its route. It is possible that civet removes this automatically before it gets to conduit's routing logic, and that hyper leaves the extra `/` intact, causing the routing logic to fail to find the correct handler. Co-authored-by: Justin Geibel <[email protected]>
This is a work in progress implementation switching from
civettohyperas the base http server.This is based on
conduit-hyperwhich I've updated tohyper 0.11. That source code is here. A huge shout-out to @sfackler and @drbawb as this is forked from their repos. I'm curios if either of you are still using this code and if you have any feedback. I plan to eventually publish this crate on crates.io, so please let me know if you would like to be added/removed as an author or if you have any objections.There are several things I wish to investigate further, including:
catch_unwind, so I'm assuming panics need to be handled in this layer.Request::remote_addr()is alwaysNonein server hyperium/hyper#1410 (comment)