-
Notifications
You must be signed in to change notification settings - Fork 950
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
tokio-based tcp transports panic when dialing due to threadpools with unset runtimes. #2230
Comments
Note: I think this only applies to people who use a tokio transport and don't use the SwarmBuilder api directly. |
Support for tokio was initially not there but was added later because of popular demand. |
Great bug report @mental32. Very much appreciate the level of detail.
Off the top of my head having the transport provide the executor is a violation of concerns. I.e. I would not expect a transport to decide on the executor to be used. In addition, given that a transport could potentially wrap multiple other transports, choosing the right executor would not be trivial.
While not ideal, this would be my preference. With the above in mind, what do you think @mental32? Do you have some time to send in a documentation patch? |
@tomaka @mxinden thanks for your insights! TBH I think it'd be best if there was a section in the docs for @mxinden I'll fork, add some doc changes, and PR something in soon-ish 👍 |
There is also a proposal in #2173 to introduce features on the |
We might want to consider not using a Lines 1335 to 1348 in 6cc3b4e
We could instead use a |
I think I would prefer a type-safe builder pattern that forces you to choose between calling |
I am closing this in favor of tracking it in #3068. |
Previously, the executor for connection tasks silently defaulted to a `futures::executor::ThreadPool`. This causes issues such as #2230. With this patch, we force the user to choose, which executor they want to run the connection tasks on which results in overall simpler API with less footguns. Closes #3068.
Previously, the executor for connection tasks silently defaulted to a `futures::executor::ThreadPool`. This causes issues such as libp2p#2230. With this patch, we force the user to choose, which executor they want to run the connection tasks on which results in overall simpler API with less footguns. Closes libp2p#3068.
I've spent the last couple of hours trying to figure this out but the best I can boil it down into is:
libp2p::tokio_development_transport
libp2p::swarm::Swarm::new
ends up using theDefault
constructed fields and callsSwarmBuilder::build
Default
constructedNetworkConfig
which sets the executor toNone
a call is made toor_with_executor(f)
to construct a futures::executor, ThreadPool as the backing executor.Swarm::dial_addr
) will eventually lead to a panic because:TcpStream
which at some point callstokio::io::PollEvented::new_with_interest
tokio::Handle::current
and that usestokio::runtime::context::io_handle()
"there is no reactor running, must be called from the context of a Tokio 1.x runtime"
Here are my deps:
Potential solutions I can think of are:
Ask the transport for an executor during
SwarmBuilder::build
so tokio transports can produce a well-configured executor and the default would just be the futures ThreadPool executor closure that is present now.Document that tokio transports must set their own executor manually using
SwarmBuilder
Expected behavior
The swarm can dial the supplied Multiaddr.
Actual behavior
Tokio panics because of an unset thread local deep in the io machinery, the context required to access a runtime.
Reproducible example
Directory layout:
src/main.rs
/cargo.toml
Output & Backtrace
Client A with backtrace enabled:
What it looks like from Client B:
The text was updated successfully, but these errors were encountered: