Skip to content

Make proc-macro bidirectional calls cancellation safe#21410

Merged
Veykril merged 11 commits intorust-lang:masterfrom
Shourya742:2026-01-06-improve-bidirectional-cancellation
Feb 1, 2026
Merged

Make proc-macro bidirectional calls cancellation safe#21410
Veykril merged 11 commits intorust-lang:masterfrom
Shourya742:2026-01-06-improve-bidirectional-cancellation

Conversation

@Shourya742
Copy link
Copy Markdown
Member

Catch panics in subrequest callbacks, reply with Cancel, and propagate cancellation to the proc-macro server

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 6, 2026
Copy link
Copy Markdown
Contributor

@ChayimFriedman2 ChayimFriedman2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few comments.

Comment thread crates/proc-macro-api/src/bidirectional_protocol.rs
Comment thread crates/proc-macro-srv-cli/src/main_loop.rs
Comment thread crates/proc-macro-srv/src/lib.rs Outdated
fn handle_failure(failure: Result<bidirectional::SubResponse, ProcMacroClientError>) -> ! {
match failure {
Err(ProcMacroClientError::Cancelled { reason }) => {
panic_any(ProcMacroCancelMarker { reason });
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
panic_any(ProcMacroCancelMarker { reason });
resume_unwind(ProcMacroCancelMarker { reason });

There is no need to incur the panic handler in this case

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we are not continuing an existing panic. If something goes wrong on the client side, we catch the panic there and convert it into a cancellation message. Up to this point, there is no panic on the server side. We then intentionally originate a new panic here with a marker, which is later caught at the thread join boundary.

Copy link
Copy Markdown
Member

@Veykril Veykril Jan 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do want to resume_unwind this as otherwise on every cancellation in this flow we will print the backtrace. resume_unwind is really just panicking but without invoking the panic handler. This is really more us trying to bubble up an error in this case without being noisy about it. note that I am only talking about this branch, the other two remaining panics is fine.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh!!! I understand it better now, thanks for clarification.

Comment thread crates/proc-macro-srv-cli/src/main_loop.rs
Comment thread crates/proc-macro-srv/src/lib.rs Outdated
Comment thread crates/proc-macro-srv/src/lib.rs
Copy link
Copy Markdown
Contributor

@ChayimFriedman2 ChayimFriedman2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@Shourya742 Shourya742 force-pushed the 2026-01-06-improve-bidirectional-cancellation branch 4 times, most recently from 711aae6 to b2cf390 Compare January 11, 2026 11:11
@Shourya742 Shourya742 requested a review from Veykril January 11, 2026 13:06
@Shourya742 Shourya742 force-pushed the 2026-01-06-improve-bidirectional-cancellation branch from e32cc52 to f303bd5 Compare February 1, 2026 09:49
@Veykril Veykril added this pull request to the merge queue Feb 1, 2026
Merged via the queue into rust-lang:master with commit 1cf041f Feb 1, 2026
15 checks passed
@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants