Update tokio-macros to new builder #2906

Darksonn · 2020-10-03T18:31:53Z

Since the builder is not yet fully updated, this still generates code with the old builder syntax, but this is easily changed later. The rt-threaded feature is also not renamed to rt-work-stealing, but this is also easily changed later. Documentation is still a TODO.

LucioFranco

Thanks a ton for getting this started!

LucioFranco · 2020-10-05T15:49:40Z

tokio-macros/src/entry.rs

+            "in_place" => Ok(RuntimeFlavor::InPlace),
+            "work_stealing" => Ok(RuntimeFlavor::WorkStealing),
+            "single_threaded" => Err(format!("The single threaded runtime flavor is called `in_place`.")),
+            "basic_scheduler" => Err(format!("The `basic_scheduler` runtime flavor has been renamed to `in_place`.")),


do we need to do this? I'd rather not leave "old" stuff in a breaking change.

No. I added it to ease the transition for users coming from v0.2, and because it wouldn't be a breaking change to remove later.

Hmm this will just generate a compile failure right? Then I think that is fine.

LucioFranco · 2020-10-05T15:50:23Z

tokio-macros/src/entry.rs

+            "single_threaded" => Err(format!("The single threaded runtime flavor is called `in_place`.")),
+            "basic_scheduler" => Err(format!("The `basic_scheduler` runtime flavor has been renamed to `in_place`.")),
+            "threaded_scheduler" => Err(format!("The `threaded_scheduler` runtime flavor has been renamed to `work_stealing`.")),
+            _ => Err(format!("No such runtime flavor `{}`.", s)),


Should we list the valid options here?

LucioFranco · 2020-10-05T15:51:13Z

tokio-macros/src/entry.rs

+    num_workers: Option<(usize, Span)>,
+    max_threads: Option<(usize, Span)>,
+}
+impl Configuration {


Some new lines in this file would be nice 😄

LucioFranco · 2020-10-05T15:53:26Z

tokio-macros/src/entry.rs

+                        "The `num_workers` option requires the `work_stealing` runtime flavor."
+                    ));
+                }
+                assert!(self.num_workers.is_none());


can we statically ensure this somehow? Maybe via enum values?

We could certainly statically ensure it in the FinalConfig, but that wouldn't remove this assert. I suppose we could turn Configuration into a state machine of some sort.

Though turning Configuration into a state machine is awkward. I chose the current design because it allows e.g. setting num_workers first, then setting the flavor.

Here's one option:

match (flavor, self.num_workers) { (CurrentThread, Some((_, num_workers_span))) => { Err(syn::Error::new( num_workers_span.clone(), "The `num_workers` option requires the `threaded` runtime flavor." )) }, (CurrentThread, None) => { Ok(FinalConfig { flavor, num_workers: None, }) }, (Threaded, num_workers) if self.rt_threaded_available => { Ok(FinalConfig { flavor, num_workers: num_workers.map(|(val, _span)| val), }) }, (Threaded, _) => { let msg = if self.flavor.is_none() { "The default runtime flavor is `threaded`, but the `rt-threaded` feature is disabled." } else { "The runtime flavor `threaded` requires the `rt-threaded` feature." }; Err(syn::Error::new(Span::call_site(), msg)) }, }

LucioFranco · 2020-10-05T15:55:47Z

tokio-macros/src/entry.rs

+                    (None, Some((_, span))) => {
+                        return Err(syn::Error::new(
+                                span,
+                                "`num_workers` must also be set when using `max_threads`"


not following why this is required?

LucioFranco · 2020-10-05T15:56:35Z

tokio-macros/src/entry.rs

+                    (Some((num_workers, _)), Some((max_threads, span))) if max_threads <= num_workers => {
+                        return Err(syn::Error::new(
+                                span,
+                                "`max_threads` must be larger than `num_workers`",


In this case, can we just default to max(core_threads, max_threads)? I think that is a better default and I think we should do the same in the builder.

Allowing max_threads == core_threads seems dangerous and like an edge case that I'm not sure we should support from the macro.

we could do max_threads = core_threads + 1 at a min. That said, lets not do this here since I think this could also be a potential footgun. Honestly, should we even allow someone to set max threads here?

LucioFranco · 2020-10-05T15:57:28Z

tokio-macros/src/entry.rs

                    },
+                    "core_threads" => {


if we are renaming lets just drop it.

LucioFranco · 2020-10-05T15:58:32Z

tokio-macros/src/lib.rs

+pub fn main_fail(_args: TokenStream, _item: TokenStream) -> TokenStream {
+    syn::Error::new(
+        proc_macro2::Span::call_site(),
+        "The #[tokio::main] macro requires rt-core or rt-threaded.",


should we link to docs here?

LucioFranco · 2020-10-05T15:59:09Z

tokio/tests/task_blocking.rs

@@ -28,7 +28,7 @@ async fn basic_blocking() {
    }
 }

-#[tokio::test(threaded_scheduler)]
+#[tokio::test(flavor = "work_stealing")]


personally, I prefer the old method rather than this change.

LucioFranco · 2020-10-07T15:05:32Z

@Darksonn looks like some CI errors?

Darksonn · 2020-10-08T07:35:03Z

I managed to make it compile.

carllerche · 2020-10-12T03:39:34Z

Thanks 👍 I merged this into #2876. I am going to close this PR.

Darksonn added 2 commits October 3, 2020 20:26

Rewrite tokio-macros to new argument format

1db81a9

Update tests

1ecd3bc

Darksonn added C-enhancement Category: A PR with an enhancement or bugfix. A-tokio-macros Area: The tokio-macros crate A-tokio Area: The main tokio crate M-macros Module: macros in the main Tokio crate labels Oct 3, 2020

Darksonn requested a review from LucioFranco October 3, 2020 18:31

Darksonn self-assigned this Oct 3, 2020

LucioFranco requested changes Oct 5, 2020

View reviewed changes

Darksonn added 2 commits October 5, 2020 18:31

Rename to current_thread/threaded and remove max_threads

ccd426d

Use match in build

019945f

Darksonn added 6 commits October 7, 2020 18:13

Fix tests

8c2b54a

rustfmt

a1ae97c

clippy

430993d

Try to fix doctests

10a2735

test_fail as main -> test

0775bd4

Remove shell runtime test

97717ac

Darksonn mentioned this pull request Oct 10, 2020

rt: Remove threaded_scheduler() and basic_scheduler() #2876

Merged

carllerche closed this Oct 12, 2020

Darksonn deleted the macro-new-builder branch October 12, 2020 07:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update tokio-macros to new builder #2906

Update tokio-macros to new builder #2906

Darksonn commented Oct 3, 2020

LucioFranco left a comment

LucioFranco Oct 5, 2020

Darksonn Oct 5, 2020

LucioFranco Oct 5, 2020

LucioFranco Oct 5, 2020

LucioFranco Oct 5, 2020

LucioFranco Oct 5, 2020

Darksonn Oct 5, 2020

Darksonn Oct 5, 2020

Darksonn Oct 5, 2020

LucioFranco Oct 5, 2020

LucioFranco Oct 5, 2020

Darksonn Oct 5, 2020

LucioFranco Oct 5, 2020

LucioFranco Oct 5, 2020

LucioFranco Oct 5, 2020

LucioFranco Oct 5, 2020

LucioFranco commented Oct 7, 2020

Darksonn commented Oct 8, 2020

carllerche commented Oct 12, 2020

Update tokio-macros to new builder #2906

Update tokio-macros to new builder #2906

Conversation

Darksonn commented Oct 3, 2020

LucioFranco left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucioFranco commented Oct 7, 2020

Darksonn commented Oct 8, 2020

carllerche commented Oct 12, 2020