Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

macros: allow setting unhandled_panic behavior in tokio::{main, test} #6593

Merged
merged 5 commits into from
Jun 7, 2024

Conversation

name1e5s
Copy link
Contributor

Motivation

Allow users to set unhandled_panic behavior in those macros directly.

Close #6527.

Solution

This PR parses unhandled_panic in tokio-macros.

@name1e5s name1e5s force-pushed the feat/macro_unhandled_panic branch 2 times, most recently from 0c0a796 to 70c7c17 Compare May 28, 2024 16:07
@Darksonn Darksonn added the A-tokio-macros Area: The tokio-macros crate label May 28, 2024
Copy link
Member

@mox692 mox692 left a comment

Choose a reason for hiding this comment

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

I would like to see the documentation for this. Perhaps it will be placed tokio-macros/src/lib.rs, there should already be similar docs there.

tokio/tests/macros_test.rs Outdated Show resolved Hide resolved
@name1e5s
Copy link
Contributor Author

I would like to see the documentation for this. Perhaps it will be placed tokio-macros/src/lib.rs, there should already be similar docs there.

Thanks for your suggestion. Documantation is added now.

@name1e5s name1e5s force-pushed the feat/macro_unhandled_panic branch from b044fb8 to d1e103b Compare May 30, 2024 08:56
Copy link
Member

@mox692 mox692 left a comment

Choose a reason for hiding this comment

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

Overall looks good to me, I left some comments regarding the docs.

tokio-macros/src/lib.rs Outdated Show resolved Hide resolved
tokio-macros/src/lib.rs Outdated Show resolved Hide resolved
@name1e5s name1e5s force-pushed the feat/macro_unhandled_panic branch 3 times, most recently from 639b69f to d8f02ad Compare June 2, 2024 14:36
@name1e5s name1e5s force-pushed the feat/macro_unhandled_panic branch from d8f02ad to c886cde Compare June 2, 2024 14:57
///
/// This option is only compatible with the `current_thread` runtime.
///
/// ```ignore
Copy link
Member

Choose a reason for hiding this comment

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

For consistency with the other examples on this page, can you use no_run instead of ignore?
(The ignore annotation makes the doc a bit scary.)
Screen Shot 2024-06-03 at 19 57 29

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For consistency with the other examples on this page, can you use no_run instead of ignore? (The ignore annotation makes the doc a bit scary.) Screen Shot 2024-06-03 at 19 57 29

I tried and found it break those CI jobs running cargo test without RUSTFLAGS="--cfg tokio_unstable", such as this link. Since we can't use conditional compilation tricks on examples, I think we can only use ignore here to make CI happy.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure that #[cfg(tokio_unstable)] works perfectly fine inside an example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm pretty sure that #[cfg(tokio_unstable)] works perfectly fine inside an example.

Yes, it works fine. I mean we can not use words like

```#[cfg(tokio_unstable)]

to disable an example entirely. But your comment reminds me that we can use # lines to make the example works under cargo test without tokio_unstable , I will try it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm pretty sure that #[cfg(tokio_unstable)] works perfectly fine inside an example.

Yes, it works fine. I mean we can not use words like

```#[cfg(tokio_unstable)]

to disable an example entirely. But your comment reminds me that we can use # lines to make the example works under cargo test without tokio_unstable , I will try it.

All tests looks fine now.

@name1e5s name1e5s force-pushed the feat/macro_unhandled_panic branch 2 times, most recently from c546470 to c886cde Compare June 5, 2024 14:48
Copy link
Member

@mox692 mox692 left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks.

@name1e5s name1e5s force-pushed the feat/macro_unhandled_panic branch 2 times, most recently from bc41125 to 6d46e25 Compare June 6, 2024 14:22
@name1e5s name1e5s force-pushed the feat/macro_unhandled_panic branch from 6d46e25 to 30d9d22 Compare June 6, 2024 14:24
@mox692 mox692 merged commit 833ee02 into tokio-rs:master Jun 7, 2024
77 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio-macros Area: The tokio-macros crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow setting unhandled_panic behavior as option on tokio::test
3 participants