feat: cancelable crate#465
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a new workspace crate, cancelable, providing C#-style cooperative cancellation primitives (token + source) plus a Future combinator to surface cancellation as a typed error.
Changes:
- Introduces
cancelablecrate withCancellationToken,CancellationTokenSource, and parent-linked cancellation. - Adds
WithCancellationExt::with_cancellationto wrap futures and returnResult<_, Canceled>when canceled. - Wires the new crate into workspace metadata (workspace deps, root README/CHANGELOG, lockfile) and adds crate assets (logo/favicon).
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Adds cancelable to the workspace crate list. |
| CHANGELOG.md | Adds cancelable to the top-level changelog index. |
| Cargo.toml | Registers cancelable as a workspace dependency entry. |
| Cargo.lock | Adds the new cancelable package entry and deps. |
| crates/cancelable/Cargo.toml | Defines the new crate package metadata and dependencies. |
| crates/cancelable/CHANGELOG.md | Adds an initial changelog stub for the new crate. |
| crates/cancelable/README.md | Adds crate-level README content (doc2readme output). |
| crates/cancelable/logo.png | Adds crate logo via Git LFS. |
| crates/cancelable/favicon.ico | Adds crate favicon via Git LFS. |
| crates/cancelable/src/lib.rs | Crate root docs and exports for token/source and future extension trait. |
| crates/cancelable/src/token.rs | Implements token/source state, subscriptions, and linked sources (+ tests). |
| crates/cancelable/src/future.rs | Implements with_cancellation future wrapper and Canceled error (+ tests). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #465 +/- ##
========================================
Coverage 100.0% 100.0%
========================================
Files 335 337 +2
Lines 25586 26131 +545
========================================
+ Hits 25586 26131 +545 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
|
I'm not particularly fond of the naming and structure of this, is there a reason we don't align this more with how Tokio does it? |
sandersaares
left a comment
There was a problem hiding this comment.
Looks nice! A lot simpler than I feared it would be and I like how well it enables .NET parity to be modeled. The subscriber leak that Copilot raised is a concern, though.
1. Disable miri for tokio tests (not supported) 2. Mutation testing fixes. Tests were hanging, but now time out and fail after 5 secs, giving them plenty of time to stabilize (e.g. not flaky). 3. Rename future extension to CancelableExt with method cancelable() and state struct Cancelable. 4. Document atomic ordering decisions 5. Propagate poisoned lock errors as panics 6. Avoid boxing when using the subscriber list to capture linked token relationships. 7. Unregister linked child token sources from their parents on Drop. 8. Update doc comments
ralfbiedert
left a comment
There was a problem hiding this comment.
Still same comment I left earlier. I think this needs a motivation why it looks like the C# version, and not like Tokio's flavor of that. In the end we try to solve Rust problems, not C# problems.
I don't know Tokio well enough to know what this means. Can you give some examples of things you'd like changed? Is it runtime differences? Naming? Different data modeling? Contractual differences? |
Tokio has a CancellationToken in the My criticism is that this API looks like a port from C#, for example having CT and CTS, how linked tokens are created, ... If at the end of a design analysis this is still the best API then of course it's fine (although CTS is too much of a name for Rust IMO), but I haven't seen any conversation where this was compared against Tokio's or any other Rust-based approach. Very generally, the first reflex for crates like this should be looking into "how does Rust solve this C# problem", not "how does C# solve this C# problem". What would be good is looking at what crates already exist out there. There is at least Tokio's, maybe |
|
tokio_util::CancellationToken looks a lot like what I've got here for my CancellationToken. APIs even have mostly the same names. I don't have a drop-guard, but that can be added to my design if it is needed. So I think what you're hinting at is that you don't like the idea of a CancellationTokenSource. This exists because I offer more than Tokio's solution -- I let callers register for callbacks so they can define their own cancellation logic rather than embedding it into the task being canceled. It's a different approach. That's why it is a bit different. I'd argue this does in fact solve a big Rust problem, and Tokio's cancellation token is evidence that something like this is needed. My impl is different, which is one reason it exists. Further, though, and more importantly, the problem of code translation from C# to Rust is front and center for Microsoft. It is a worthy problem to solve, and this helps with that, giving developers and AI agents something very familiar to work with - something missing from the Rust ecosystem. |
|
@ralfbiedert I'm approving to unblock @afoxman for now. If you're game, I think we should have a deeper discussion about your concerns here, and what recommendations you have for the API. |
|
It sounds like this needs more higher-level discussion, so I'm going to land it in an internal repo for now. After we have some consensus on how to proceed, I can get this PR landed here (with any needed changes). |
This PR introduces a new crate named
cancelable.It models C# cancelation tokens and sources in Rust, aimed at improving the accuracy of C# -> Rust code translation.