-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Consolidate errors for context missing #3441
Conversation
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Co-authored-by: Alice Ryhl <alice@ryhl.io>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
@Darksonn tried to address your feedback. |
tokio/src/io/driver/mod.rs
Outdated
crate::runtime::context::io_handle() | ||
.expect("there is no reactor running, must be called from the context of Tokio runtime") | ||
crate::runtime::context::io_handle().expect("io not enabled on Tokio context") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error should check whether there is a runtime context or not. If there is, it should fail with an error saying that there is a runtime, but IO is disabled on it. Otherwise it should fail with the same error as the other panics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Darksonn I made runtime::context::io_handle()
fail with the standard error if the Tokio context isn't instantiated, does this address your concern or should it be implemented differently?
If runtime::context::io_handle()
shouldn't panic in case of missing Tokio context, the return value has to differentiate between missing context and io being disabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't tell there being any technical drawback to have runtime::context::io_handle()
panic if the Tokio context is missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should panic in both cases, but the error messages should be different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Darksonn I'm wondering if there might be a misunderstanding; the way I've implemented it, there are different error messages depending on whether the Tokio context is missing or io isn't enabled on it.
I implemented it so that runtime::context::io_handle
will panic if the context is missing. Otherwise, io::driver::Handle::current
will panic if the context doesn't have io enabled.
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want a test for each of these cases:
- A timer was used, but no runtime is present.
- A timer was used, but the runtime has timers disabled.
- IO was used, but not runtime is present.
- IO was used, but the runtime has IO disabled.
You only have some of these cases.
tokio/src/time/driver/handle.rs
Outdated
@@ -71,8 +71,7 @@ cfg_not_rt! { | |||
/// lazy, and so outside executed inside the runtime successfuly without | |||
/// panicking. | |||
pub(crate) fn current() -> Self { | |||
panic!("there is no timer running, must be called from the context of Tokio runtime or \ | |||
`rt` is not enabled") | |||
panic!("`rt` must be enabled") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be using the standardized error message, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Darksonn Isn't the difference here that the rt
feature is disabled? It isn't just that the context hasn't been instantiated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, here the rt
feature needs to be enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Darksonn but you're suggesting this error message should change? I'm not following.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you provide a code change suggestion, if you have something in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure.
panic!(context_missing_error(&["rt"]));
In fact, you do not currently use the list argument anywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Darksonn OK, I figured it was kind of pointless to produce an error saying that there's no reactor running since the runtime isn't compiled in in the first place (due to the "rt" feature being disabled).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are free to pick another wording, but "rt must be enabled" is too short. It needs to mention that this is a Tokio feature, including the Tokio version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Darksonn Please see my updated error string.
Co-authored-by: Alice Ryhl <alice@ryhl.io>
Co-authored-by: Alice Ryhl <alice@ryhl.io>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
@Darksonn I should have tests for the first two cases. How can I write corresponding tests for io? I'm not sure how to trigger the panic wrt. io. |
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
@Darksonn Made it into a constant string. |
tokio/src/io/driver/mod.rs
Outdated
@@ -274,7 +273,7 @@ cfg_not_rt! { | |||
/// This function panics if there is no current reactor set, or if the `rt` | |||
/// feature flag is not enabled. | |||
pub(super) fn current() -> Self { | |||
panic!("there is no reactor running, must be called from the context of Tokio runtime with `rt` enabled.") | |||
panic!("feature `rt` must be enabled on Tokio 1.x") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I think using the standardized error message here too would be fine. I mean sure, you need to enable rt
to fix this error, but the root of the problem is that there is no runtime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Darksonn OK, I might do that. I kind of see it the other way around though I guess, i.e. that the root of problem is that the "rt" feature is disabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, if you enable the rt
feature, you're just going to get the other error about a runtime missing. 🤷♀️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. I'll tweak the message when I finish work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
tokio/src/time/driver/handle.rs
Outdated
@@ -71,8 +71,7 @@ cfg_not_rt! { | |||
/// lazy, and so outside executed inside the runtime successfuly without | |||
/// panicking. | |||
pub(crate) fn current() -> Self { | |||
panic!("there is no timer running, must be called from the context of Tokio runtime or \ | |||
`rt` is not enabled") | |||
panic!("feature `rt` must be enabled on Tokio 1.x") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here as before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
tokio/src/io/driver/mod.rs
Outdated
@@ -274,7 +273,7 @@ cfg_not_rt! { | |||
/// This function panics if there is no current reactor set, or if the `rt` | |||
/// feature flag is not enabled. | |||
pub(super) fn current() -> Self { | |||
panic!("there is no reactor running, must be called from the context of Tokio runtime with `rt` enabled.") | |||
panic!(&format!("{} with feature `rt` enabled", crate::util::error::CONTEXT_MISSING_ERROR)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still, I mean, you can't have a runtime without enabling rt
, but ok. This is fine with me.
panic!(&format!("{} with feature `rt` enabled", crate::util::error::CONTEXT_MISSING_ERROR)) | |
panic!("{} with feature `rt` enabled", crate::util::error::CONTEXT_MISSING_ERROR) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Darksonn I actually didn't think about that, switched to just crate::util::error::CONTEXT_MISSING_ERROR
instead.
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Co-authored-by: Alice Ryhl <alice@ryhl.io>
@Darksonn committed your suggestions. |
We have some tests to update. |
Ah yeah of course. Will take care of it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR!
Motivation
Standardize the error messages for when the Tokio context hasn't been instantiated or required properties on the context are disabled, fixes #3361.
Solution
The solution wrt. the context not being instantiated is to add constant
util::error::CONTEXT_MISSING_ERROR
, that is used instead of hardcoding the Tokio context missing error message.Happy for any feedback, e.g., if I overlooked something.