Repository navigation
Conversation
|
| Project | cot |
| Branch | build/remove-axum |
| Testbed | github-ubuntu-latest |
Click to view all benchmark results
| Benchmark | Latency | Benchmark Result microseconds (µs) (Result Δ%) | Upper Boundary microseconds (µs) (Limit %) |
|---|---|---|---|
| empty_router/empty_router | 📈 view plot 🚷 view threshold | 7,267.80 µs(-40.89%)Baseline: 12,296.05 µs | 18,508.91 µs (39.27%) |
| json_api/json_api | 📈 view plot 🚷 view threshold | 730.37 µs(-27.45%)Baseline: 1,006.77 µs | 1,345.38 µs (54.29%) |
| nested_routers/nested_routers | 📈 view plot 🚷 view threshold | 715.76 µs(-24.74%)Baseline: 951.05 µs | 1,254.98 µs (57.03%) |
| single_root_route/single_root_route | 📈 view plot 🚷 view threshold | 672.27 µs(-26.14%)Baseline: 910.18 µs | 1,209.60 µs (55.58%) |
| single_root_route_burst/single_root_route_burst | 📈 view plot 🚷 view threshold | 13,238.00 µs(-21.26%)Baseline: 16,811.28 µs | 22,263.31 µs (59.46%) |
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
As a test to ensure that the change works correctly, I'm now running https://mackow.ski with this patch (m4tx/m4txblog#70). |
There was a problem hiding this comment.
🟡 Changes recommended
The new server loop leaves the listener open during shutdown and does not preserve HTTP upgrade support.
2 open issues
What changed in this PR
Replaces Axum with Hyper for HTTP serving and decouples cot-core request bodies from either server library, addressing #182 and reducing build dependencies.
Changes:
- Adds a Hyper accept loop with graceful shutdown.
- Uses type-erased incoming bodies and native Cot responses.
- Updates dependencies and framework documentation.
| File | Description |
|---|---|
| docs/framework-comparison.md | Describes Hyper/Tokio foundations. |
| cot/src/project.rs | Replaces Axum serving and response adapters. |
| cot/src/lib.rs | Updates framework foundation documentation. |
| cot/src/error_page.rs | Returns native Cot responses. |
| cot/Cargo.toml | Replaces Axum with Hyper dependencies. |
| cot-core/src/body.rs | Type-erases incoming request bodies. |
| cot-core/Cargo.toml | Removes Axum and enables Tower utilities. |
| Cargo.toml | Updates shared server dependencies. |
| Cargo.lock | Removes unused dependency entries. |
Review effort: Balanced
81784f3 to
f171f06
Compare
fae8e49 to
391d76c
Compare
seqre
left a comment
There was a problem hiding this comment.
Overall, great work! I have only few minor questions
| B::Error: Into<Box<dyn StdError + Send + Sync>>, | ||
| { | ||
| Self::new(BodyInner::Incoming(SyncWrapper::new( | ||
| inner.map_err(Into::into).boxed_unsync(), |
There was a problem hiding this comment.
Why is boxed_unsync() needed here?
There was a problem hiding this comment.
This is mostly to avoid having to tie cot_core directly to hyper. It allows for full type erasure, and while it comes with some performance cost, considering the overhead of processing HTTP requests, it's tiny. This is exactly what we used to have, because Axum does the same thing internally: https://docs.rs/axum-core/0.5.6/src/axum_core/body.rs.html#39
But technically we could directly use hyper::Incoming here. There will most likely be a negligible improvement in performance/memory usage at the downside of having to build hyper before cot_core. Considering pure hyper builds much faster than the entire axum, I think it's reasonable to take either approach.
Do you have any preferences?
| cot_core::headers::HTML_CONTENT_TYPE, | ||
| ) | ||
| .body(axum::body::Body::new(error_str)) | ||
| .body(Body::fixed(error_str)) |
There was a problem hiding this comment.
Is this unrelated fix? This could have been done earlier
There was a problem hiding this comment.
This is not really an unrelated change. Previously, the error handling code used to produce axum responses directly to avoid having to do a fallible conversion between our response type and axum's. Now the body types is unified across the entire project, because hyper itself doesn't enforce any particular body type, unlike Axum. This is just a nice cleanup as a side effect of the change.
| let handler = handler.clone(); | ||
| let service = hyper::service::service_fn(move |request: http::Request<Incoming>| { | ||
| let handler = handler.clone(); |
There was a problem hiding this comment.
One is for each connection, and the other is for each request within that connection. It seems like at least the outer one (per connection) can be replaced with Arc, making this cheaper. Done in the latest commit. The inner one must stay, as the handler is FnOnce, so we need a separate instance for every call.
Double clone is what Axum is doing internally anyway, so this won't be any slower than the old version.
| // other errors (e.g. `EMFILE` when the process has hit the limit of open | ||
| // files) are likely not going to go away immediately, so we wait a bit | ||
| // instead of spinning in a loop. This is the same strategy as used by | ||
| // axum. | ||
| error!(?error, "failed to accept connection"); | ||
| tokio::time::sleep(Duration::from_secs(1)).await; |
There was a problem hiding this comment.
It doesn't have to happen now (issue for future would be enough), but this is something I'd like to be configurable
Cot only used axum for `axum::serve` and its body type, so this replaces it with a small accept loop built on hyper and hyper-util. cot_core no longer depends on axum (nor hyper) at all; incoming request bodies are stored as a type-erased `http_body::Body`. This removes axum from the critical path of a cold build, as cot_core doesn't need to wait for axum/hyper to be compiled anymore. Note that axum-core is still pulled in by tower-session. This is blocked on a tower-sessions change: maxcountryman/tower-sessions#270.
b982ea9 to
1896167
Compare

Related issue or discussion
Fixes #182.
Description
Cot only used axum for
axum::serveand its body type, so this replaces it with a small accept loop built on hyper and hyper-util.cot_core no longer depends on axum (nor hyper) at all; incoming request bodies are stored as a type-erased
http_body::Body. This removes axum from the critical path of a cold build, as cot_core doesn't need to wait for axum/hyper to be compiled anymore.Note that axum-core is still pulled in by tower-session. This is blocked on a tower-sessions change: maxcountryman/tower-sessions#270.
On my machine (16-thread AMD Ryzen 7 PRO 7840U, 64GB of RAM, power saving mode) and --no-default-features, the build goes from 25.4sec to 22.1sec (so around 13% saving). The saving should be pretty consistent even with a lot of CPU cores, because axum was on the critical build path.
Type of change
Checklist
just test-all)just clippy)cargo fmt)