add a Handle::current() for getting access to TLS Runtime #2040

bluejekyll · 2019-12-29T20:53:29Z

Motivation

Get a Runtime::Handle to the current thread local storage Runtime components.

Solution

This adds a blocking::Spawner reference to ThreadContext. With that we then have access to all the components needed for a Handle::current. This will panic if components haven't been registered that are required.

bluejekyll · 2019-12-29T23:49:26Z

tokio/src/runtime/blocking/mod.rs

@@ -60,6 +60,8 @@ cfg_not_blocking_impl! {
        where
            F: FnOnce() -> R,
        {
+            let ctx = crate::runtime::context::ThreadContext::clone_current();


I don't see why this isn't necessary and wasn't here before...

Currently, since BlockingPool::enter only sets the context for the blocking pool spawner, the no-op version doesn't need to do anything. This fn is only here to make feature flag code easier.

Ok. Would you like me to remove this?

I think removing it would be best.

bluejekyll · 2019-12-29T23:51:51Z

tokio/src/runtime/blocking/pool.rs

-                }
-            }
+        let ctx = crate::runtime::context::ThreadContext::clone_current();
+        let _e = ctx.with_blocking_spawner(self.clone()).enter();


I'm confused about why the Cell with a raw pointer was necessary here, when Spawner has an internal Arc.

So I simplified this to focus on just using that fact. I'm sure I'm missing something. The Drop for ThreadContext along with enter performs the same revert that was happening with the Reset before.

carllerche · 2020-01-02T19:46:30Z

tokio/src/runtime/context.rs

+        feature = "io-std",
+        feature = "rt-threaded",
+    ))]
+    pub(crate) fn with_blocking_spawner(


I think this can be replaced w/ the cfg_blocking_impl macro (here). It cannot go around fn defs, so you would split it out to another impl block:

cfg_blocking_impl! { impl ThreadContext { pub(crate) fn with_blocking_spawner(....) { ... } } }

carllerche · 2020-01-02T19:47:07Z

tokio/src/runtime/context.rs

    pub(crate) fn io_handle() -> crate::runtime::io::Handle {
+        #[cfg(any(not(feature = "io-driver"), loom))]


Could you elaborate on why this change was made?

Oh. I think it was failing CI with a unused warning? Though I can try reverting it.

Should the fn io_handle be scoped w/ a cfg then? The fns below have had their cfg bits removed. I'd be interested in seeing the error.

So the way that it's written right now, the field is not optional, it gets swapped out for () when these features aren't enabled (apparently). Here's the error with a default cargo check with those changed reverted::

error[E0308]: match arms have incompatible types --> tokio/src/runtime/context.rs:105:21 | 103 | CONTEXT.with(|ctx| match *ctx.borrow() { | ____________________________- 104 | | Some(ref ctx) => ctx.io_handle.clone(), | | --------------------- this is found to be of type `()` 105 | | None => None, | | ^^^^ expected (), found enum `std::option::Option` 106 | | }) | |_________- `match` arms have incompatible types | = note: expected type `()` found type `std::option::Option<_>`

Ah! What if you sub None for Default::default()?

That said, it is curious that io_handle() is required even when the io-driver feature isn't enabled.

This stems from the way the Handle::Current works, which asks for all the components of ThreadContext, where Handle has all of these, so cfging off the io_handle function will spider out for a lot of other changes. Maybe Default::default() will work...

Ok, that works.

tokio/src/runtime/handle.rs

carllerche · 2020-01-02T19:56:13Z

This looks good to me. I added some minor inline comments.

bluejekyll · 2020-01-03T07:26:45Z

ok, @carllerche , I think I cleaned up what could be. Let me know if you want me to take care of anything else.

carllerche · 2020-01-06T18:59:54Z

I merged master & resolved the conflict.

bluejekyll · 2020-01-06T19:06:04Z

Thanks! I didn't notice we ended up with a conflict.

carllerche · 2020-01-06T19:06:49Z

I've been merging a lot of PRs

carllerche

Thanks for pushing this through 👍

bluejekyll · 2020-01-06T19:16:59Z

No problem. It's really benefiting from all the work that was done on ThreadContext.

Thanks for managing all of this!

add a Handle::current() for getting access to TLS Runtime

628f5e1

bluejekyll closed this Dec 29, 2019

bluejekyll reopened this Dec 29, 2019

bluejekyll added 5 commits December 29, 2019 15:21

remove thread local from blocking pool

501e6f1

remove thread local in blocking

80f1705

remove outdated comment

4c40fa0

change error message to match others about Runtime not existing.

1aae563

remove unnecessary call to blocking_spawn.enter in Handle

7711bf1

bluejekyll commented Dec 30, 2019

View reviewed changes

bluejekyll mentioned this pull request Dec 30, 2019

Can runtime arguments be made optional? hickory-dns/hickory-dns#977

Closed

cleanup features on new methods

370844d

daxpedda mentioned this pull request Dec 31, 2019

Re-enable trust-dns feature seanmonstar/reqwest#618

Closed

bluejekyll mentioned this pull request Jan 2, 2020

[trust-dns-resolver] refactor unit tests hickory-dns/hickory-dns#979

Merged

carllerche reviewed Jan 2, 2020

View reviewed changes

tokio/src/runtime/handle.rs Show resolved Hide resolved

bluejekyll added 2 commits January 2, 2020 16:32

add doc test for Handle::current

92d56fa

use Default::default() instead of cfg for optional Context members

816fe2f

Merge branch 'master' into handle-current

Verified

This commit was signed with the committer’s verified signature. The key has expired.

carllerche Carl Lerche

GPG key ID: 4E7A3CC14D9966F2
Expired

Verified
Learn about vigilant mode

2d1053e

carllerche approved these changes Jan 6, 2020

View reviewed changes

carllerche merged commit 0193df3 into tokio-rs:master Jan 6, 2020

bluejekyll deleted the handle-current branch January 8, 2020 00:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add a Handle::current() for getting access to TLS Runtime #2040

add a Handle::current() for getting access to TLS Runtime #2040

bluejekyll commented Dec 29, 2019 •

edited

Loading

bluejekyll Dec 29, 2019

carllerche Jan 2, 2020 •

edited

Loading

bluejekyll Jan 2, 2020

carllerche Jan 2, 2020

bluejekyll Jan 3, 2020

bluejekyll Dec 29, 2019

carllerche Jan 2, 2020

bluejekyll Jan 3, 2020

carllerche Jan 2, 2020

bluejekyll Jan 2, 2020

carllerche Jan 2, 2020

bluejekyll Jan 2, 2020

carllerche Jan 2, 2020

bluejekyll Jan 3, 2020

bluejekyll Jan 3, 2020

carllerche commented Jan 2, 2020

bluejekyll commented Jan 3, 2020

carllerche commented Jan 6, 2020

bluejekyll commented Jan 6, 2020

carllerche commented Jan 6, 2020

carllerche left a comment

bluejekyll commented Jan 6, 2020

		pub(crate) fn io_handle() -> crate::runtime::io::Handle {
		#[cfg(any(not(feature = "io-driver"), loom))]

add a Handle::current() for getting access to TLS Runtime #2040

add a Handle::current() for getting access to TLS Runtime #2040

Conversation

bluejekyll commented Dec 29, 2019 • edited Loading

Motivation

Solution

Choose a reason for hiding this comment

carllerche Jan 2, 2020 • edited Loading

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

carllerche commented Jan 2, 2020

bluejekyll commented Jan 3, 2020

carllerche commented Jan 6, 2020

bluejekyll commented Jan 6, 2020

carllerche commented Jan 6, 2020

carllerche left a comment

Choose a reason for hiding this comment

bluejekyll commented Jan 6, 2020

bluejekyll commented Dec 29, 2019 •

edited

Loading

carllerche Jan 2, 2020 •

edited

Loading