Consolidate errors for context missing #3441

aknuds1 · 2021-01-17T11:34:22Z

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.

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

tokio/src/task/spawn.rs

tokio/src/io/driver/mod.rs

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

tokio/src/util/error.rs

Co-authored-by: Alice Ryhl <alice@ryhl.io>

…sing

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

aknuds1 · 2021-01-17T15:46:23Z

@Darksonn tried to address your feedback.

Darksonn · 2021-01-17T15:52:37Z

tokio/src/io/driver/mod.rs

-            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")


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.

@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.

I can't tell there being any technical drawback to have runtime::context::io_handle() panic if the Tokio context is missing.

It should panic in both cases, but the error messages should be different.

@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>

Darksonn

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/tests/no_rt.rs

tokio/src/time/driver/handle.rs

Darksonn · 2021-01-17T17:03:41Z

tokio/src/time/driver/handle.rs

@@ -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")


This should be using the standardized error message, no?

@Darksonn Isn't the difference here that the rt feature is disabled? It isn't just that the context hasn't been instantiated.

Yes, here the rt feature needs to be enabled.

@Darksonn but you're suggesting this error message should change? I'm not following.

Can you provide a code change suggestion, if you have something in mind?

Sure.

panic!(context_missing_error(&["rt"]));

In fact, you do not currently use the list argument anywhere.

@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).

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.

@Darksonn Please see my updated error string.

Co-authored-by: Alice Ryhl <alice@ryhl.io>

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

aknuds1 · 2021-01-17T17:21:40Z

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.

@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>

aknuds1 · 2021-01-18T08:10:39Z

@Darksonn Made it into a constant string.

tokio/src/io/driver/mod.rs

Darksonn · 2021-01-18T09:19:38Z

tokio/src/io/driver/mod.rs

@@ -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")


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.

@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.

I mean, if you enable the rt feature, you're just going to get the other error about a runtime missing. 🤷‍♀️

Yeah. I'll tweak the message when I finish work.

tokio/src/time/driver/handle.rs

Darksonn · 2021-01-18T09:20:57Z

tokio/src/time/driver/handle.rs

@@ -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")


Same here as before.

…sing

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

Darksonn · 2021-01-19T19:03:24Z

tokio/src/io/driver/mod.rs

@@ -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))


Still, I mean, you can't have a runtime without enabling rt, but ok. This is fine with me.

Suggested change

panic!(&format!("{} with feature `rt` enabled", crate::util::error::CONTEXT_MISSING_ERROR))

panic!("{} with feature `rt` enabled", crate::util::error::CONTEXT_MISSING_ERROR)

@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>

Co-authored-by: Alice Ryhl <alice@ryhl.io>

aknuds1 · 2021-01-20T11:52:10Z

@Darksonn committed your suggestions.

Darksonn · 2021-01-20T12:24:09Z

We have some tests to update.

aknuds1 · 2021-01-20T12:26:32Z

We have some tests to update.

Ah yeah of course. Will take care of it.

…sing

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

Darksonn

Thank you for the PR!

aknuds1 added 2 commits January 17, 2021 12:26

Use function to produce error string for context missing

eefa4f6

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

context::missing_error: Include Tokio version

a7b29ee

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

aknuds1 marked this pull request as ready for review January 17, 2021 11:45

aknuds1 added 6 commits January 17, 2021 12:52

context::missing_error: Hardcode Tokio version

5a45db8

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

Formatting

68ce4af

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

Simplify

ed42e6e

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

Simplify

6f90e12

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

Appease clippy

8b26582

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

Fix test errors

bf2d2fc

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

Darksonn reviewed Jan 17, 2021

View reviewed changes

tokio/src/task/spawn.rs Outdated Show resolved Hide resolved

Darksonn reviewed Jan 17, 2021

View reviewed changes

tokio/src/io/driver/mod.rs Outdated Show resolved Hide resolved

Fix test errors

61616ad

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

Darksonn reviewed Jan 17, 2021

View reviewed changes

tokio/src/util/error.rs Outdated Show resolved Hide resolved

aknuds1 and others added 3 commits January 17, 2021 14:34

Update tokio/src/util/error.rs

1f90f0e

Co-authored-by: Alice Ryhl <alice@ryhl.io>

Merge remote-tracking branch 'tokio-rs/master' into chore/context-mis…

eef23e6

…sing

Apply feedback from review

9cf7fbe

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

aknuds1 requested a review from Darksonn January 17, 2021 15:45

Darksonn reviewed Jan 17, 2021

View reviewed changes

aknuds1 added 4 commits January 17, 2021 17:12

Improve error messages

98b50b1

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

Fix tests

3126cfd

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

Fix test

b2d8eb6

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

Add integration test

18a6c13

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

Darksonn reviewed Jan 17, 2021

View reviewed changes

aknuds1 and others added 5 commits January 17, 2021 18:05

Update tokio/tests/no_rt.rs

5d67039

Co-authored-by: Alice Ryhl <alice@ryhl.io>

Update tokio/src/time/driver/handle.rs

b1f4558

Co-authored-by: Alice Ryhl <alice@ryhl.io>

Fix test

e62d4dd

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

Fix test

c707db0

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

Formatting

a1bbf25

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

Formatting

47ff6ef

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

aknuds1 changed the title ~~Use function to produce error string for context missing~~ Consolidate errors for context missing Jan 18, 2021

Darksonn reviewed Jan 18, 2021

View reviewed changes

aknuds1 added 2 commits January 19, 2021 19:49

Merge remote-tracking branch 'tokio-rs/master' into chore/context-mis…

7602e03

…sing

Fixup error messages

0e6f644

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

aknuds1 requested a review from Darksonn January 19, 2021 18:57

Darksonn reviewed Jan 19, 2021

View reviewed changes

Tweak error message

d0d7751

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

aknuds1 requested a review from Darksonn January 19, 2021 19:47

aknuds1 and others added 3 commits January 19, 2021 20:53

Enable usage of util::error without rt feature

7abf22c

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

Fix tests

3c34b95

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

Apply suggestions from code review

8adddb1

Co-authored-by: Alice Ryhl <alice@ryhl.io>

Darksonn and others added 4 commits January 20, 2021 13:33

io: keep track of initialized bytes in read_to_end (tokio-rs#3426)

c0a7f63

util: add pollable Semaphore (tokio-rs#3444)

d88b63d

Merge remote-tracking branch 'tokio-rs/master' into chore/context-mis…

9a9244e

…sing

Fix tests

f4598e8

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

Darksonn approved these changes Jan 20, 2021

View reviewed changes

Darksonn merged commit fdde558 into tokio-rs:master Jan 20, 2021

aknuds1 deleted the chore/context-missing branch January 20, 2021 13:31

dekellum mentioned this pull request Jan 22, 2021

Include Tokio version in "no such executor" for Tokio 0.2.x and 0.3.x #3442

Closed

Darksonn added A-tokio Area: The main tokio crate M-runtime Module: tokio/runtime labels Jan 22, 2021

Darksonn mentioned this pull request Jan 22, 2021

Prepare Tokio 1.1.0 #3462

Merged

dependabot bot mentioned this pull request Mar 8, 2021

Bump tokio from 0.2.25 to 1.2.0 technic/epg-server#135

Closed

dependabot bot mentioned this pull request Mar 17, 2021

Bump tokio from 0.2.23 to 1.2.0 predominant/habitat#3

Open

seunghunee mentioned this pull request Nov 17, 2021

Improve context related panic messages #2465

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consolidate errors for context missing #3441

Consolidate errors for context missing #3441

aknuds1 commented Jan 17, 2021 •

edited

aknuds1 commented Jan 17, 2021

Darksonn Jan 17, 2021

aknuds1 Jan 17, 2021

aknuds1 Jan 17, 2021

Darksonn Jan 17, 2021

aknuds1 Jan 17, 2021 •

edited

Darksonn left a comment

Darksonn Jan 17, 2021

aknuds1 Jan 17, 2021 •

edited

Darksonn Jan 17, 2021

aknuds1 Jan 17, 2021

aknuds1 Jan 17, 2021

Darksonn Jan 17, 2021

aknuds1 Jan 17, 2021

Darksonn Jan 17, 2021

aknuds1 Jan 17, 2021

aknuds1 commented Jan 17, 2021

aknuds1 commented Jan 18, 2021

Darksonn Jan 18, 2021

aknuds1 Jan 18, 2021

Darksonn Jan 18, 2021

aknuds1 Jan 18, 2021 •

edited

aknuds1 Jan 19, 2021

Darksonn Jan 18, 2021

aknuds1 Jan 19, 2021

Darksonn Jan 19, 2021

aknuds1 Jan 19, 2021 •

edited

aknuds1 commented Jan 20, 2021

Darksonn commented Jan 20, 2021

aknuds1 commented Jan 20, 2021 •

edited

Darksonn left a comment

	panic!(&format!("{} with feature `rt` enabled", crate::util::error::CONTEXT_MISSING_ERROR))
	panic!("{} with feature `rt` enabled", crate::util::error::CONTEXT_MISSING_ERROR)

Consolidate errors for context missing #3441

Consolidate errors for context missing #3441

Conversation

aknuds1 commented Jan 17, 2021 • edited

Motivation

Solution

aknuds1 commented Jan 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aknuds1 Jan 17, 2021 • edited

Choose a reason for hiding this comment

Darksonn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aknuds1 Jan 17, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aknuds1 commented Jan 17, 2021

aknuds1 commented Jan 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aknuds1 Jan 18, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aknuds1 Jan 19, 2021 • edited

Choose a reason for hiding this comment

aknuds1 commented Jan 20, 2021

Darksonn commented Jan 20, 2021

aknuds1 commented Jan 20, 2021 • edited

Darksonn left a comment

Choose a reason for hiding this comment

aknuds1 commented Jan 17, 2021 •

edited

aknuds1 Jan 17, 2021 •

edited

aknuds1 Jan 17, 2021 •

edited

aknuds1 Jan 18, 2021 •

edited

aknuds1 Jan 19, 2021 •

edited

aknuds1 commented Jan 20, 2021 •

edited