Conversation
Not requiring this probably isn't very useful (as there are most likely not many use cases where `Project` needs to contain any data; yet alone data that cannot be `Send`). With `Project: Send`, the bootstrapper is `Send` as well which is useful when you want to run the server in tokio::spawn (for instance, as part of an already running async program) instead of tokio::block_on.
|
See m4tx/tundra#173 on why this is useful. |
There was a problem hiding this comment.
Pull request overview
This PR makes Project require Send so that Bootstrapper (and related async boot/run flows) can be Send, enabling use cases like running the server inside tokio::spawn rather than requiring tokio::block_on.
Changes:
- Add
Sendas a supertrait bound onProject. - Remove now-unnecessary
clippy::future_not_sendsuppressions inBootstrapperboot/run paths and in tests/utilities. - Keep/adjust remaining clippy attributes where
unused_asyncis still intentionally expected.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| cot/src/project.rs | Makes Project: Send and removes future_not_send expectations from bootstrap/run APIs impacted by Project’s sendability. |
| cot/src/test.rs | Removes future_not_send expectation from Client::new now that Project is Send. |
| cot/src/middleware/live_reload.rs | Removes future_not_send expectation from a helper test that uses Bootstrapper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cot/src/project.rs
Outdated
| /// } | ||
| /// ``` | ||
| pub trait Project { | ||
| pub trait Project: Send { |
There was a problem hiding this comment.
Project is a public trait and adding : Send is a breaking API change for downstream implementors that were using Rc/RefCell or other !Send state. This repo’s CHANGELOG follows SemVer/Keep-a-Changelog and typically labels breaking changes; please add an entry under [Unreleased] marking this as breaking (and ensure the release/versioning process accounts for it).
| pub trait Project: Send { | |
| pub trait Project { |
|
Bencher Report| Branch | project-send |
| 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 | 5,732.00 µs(-3.09%)Baseline: 5,914.68 µs | 7,080.96 µs (80.95%) |
| json_api/json_api | 📈 view plot 🚷 view threshold | 1,011.60 µs(-0.34%)Baseline: 1,015.07 µs | 1,160.24 µs (87.19%) |
| nested_routers/nested_routers | 📈 view plot 🚷 view threshold | 940.67 µs(+0.54%)Baseline: 935.66 µs | 1,063.30 µs (88.47%) |
| single_root_route/single_root_route | 📈 view plot 🚷 view threshold | 899.94 µs(+0.37%)Baseline: 896.60 µs | 1,021.38 µs (88.11%) |
| single_root_route_burst/single_root_route_burst | 📈 view plot 🚷 view threshold | 16,065.00 µs(-8.27%)Baseline: 17,513.17 µs | 20,808.56 µs (77.20%) |
m4tx
changed the title
Project require SendProject require Send
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:
|
| @@ -2308,7 +2308,7 @@ pub(crate) fn prepare_request_for_error_handler(request_head: &mut RequestHead, | |||
| /// # } | |||
| /// ``` | |||
| #[expect(clippy::future_not_send)] // Send not needed; CLI is run async in a single thread | |||
There was a problem hiding this comment.
If we now require Send here, do we still need this clippy attribute here (as well as the comment) or am I missing something?
There was a problem hiding this comment.
Good question! This particular function is not Send because it uses CliTask, which is explicitly marked as not requiring Send:
Line 229 in 97d4b3e
I think it might still potentially be useful to create CLI tasks that contain some non-Send data, and by definition, they shouldn't be ran as part of other applications, especially in already existing async runtimes. I'm, however, happy to be wrong - if you think like there's a use case for this, then I'm happy to drop this Send requirement as well.
Not requiring this probably isn't very useful (as there are most likely not many use cases where
Projectneeds to contain any data; yet alone data that cannot beSend). WithProject: Send, the bootstrapper isSendas well which is useful when you want to run the server in tokio::spawn (for instance, as part of an already running async program) instead of tokio::block_on.EDIT: After a consideration, changed the
Bootstrapperto requireProject + Sendinstead of adding the requirement to theProjecttrait itself, which could be useful if we found a good use case for using!Sendproject in the future and wanted to use it without breaking APIs.