Skip to content
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

Upgrade to latest tokio (0.2) or switch to async-std? #1318

Closed
miketang84 opened this issue Nov 25, 2019 · 17 comments · Fixed by #1699
Closed

Upgrade to latest tokio (0.2) or switch to async-std? #1318

miketang84 opened this issue Nov 25, 2019 · 17 comments · Fixed by #1699

Comments

@miketang84
Copy link
Contributor

Hi, is there some plans to upgrade to latest tokio (0.2) or switch to async-std?

Thank you.

@mxinden
Copy link
Member

mxinden commented Nov 25, 2019

Hi @daogangtang,

We are currently updating rust-libp2p to use stable futures as well as async/await. You can follow the progress via the stable-futures branch: https://github.com/libp2p/rust-libp2p/tree/stable-futures. Most of the modules only depend on the async-std and the futures crate.

@miketang84
Copy link
Contributor Author

Good news!

@vorot93
Copy link
Contributor

vorot93 commented Dec 11, 2019

Any particular reason for choosing async-std over tokio?

@tomaka
Copy link
Member

tomaka commented Dec 11, 2019

Any particular reason for choosing async-std over tokio?

Initially it's because tokio 0.2 wasn't even a thing.
Now I'd be more keep on picking async-std because they work independently of which executor polls them (contrary to tokio sockets that need a tokio reactor to poll them).

@carllerche
Copy link

carllerche commented Dec 11, 2019

You should, of course, pick which ever option works best for your case. I will just clarify some points for future readers of this issue.

Both Tokio and async-std require a reactor to drive socket types. The primary difference is that Tokio does not implicitly start a reactor where as async-std implicitly start background threads from a static context. The advantage of not implicitly starting a reactor is that it allows the user to control how, when, and where the reactor runs (background thread, current thread, inline w/ blocking fn calls, ...).

Tokio sockets support being used from any executor, it just requires you to specify which reactor to use to bind them. Roughly, it is something like:

// start a runtime that only includes an I/O driver
let rt = runtime::Builder::new()
    .enable_io()
    .build()
    .unwrap();

let rt_handle = rt.handle().clone();

// Run the runtime somewhere (background thread?)

let listener = rt_handle.enter(|| tokio::net::TcpListener::bind(...));

This has some advantages. For example, rust-postgres has seen significant performance improvements by controlling the details of how the I/O driver runs in its sync API bridge.

@tomaka
Copy link
Member

tomaka commented Dec 11, 2019

I totally agree that tokio has theoretically better performances. What I'm trying to avoid here is the surprise factor.

I hate the situation where everything compiles just fine, but then produces runtime errors because some initialization process hasn't been performed. In particular, I've been bitten multiple times when it comes to timers, since they appear to instantly succeed if not polled through a reactor of the same tokio version.

In order to be properly documented, one would need to document rust-libp2p and every single API layer on top of it to mention "must be polled through a tokio v0.2 reactor, otherwise it will look like it works but it doesn't".

Just this morning we had a panic in Polkadot because we're mixing tokio v0.1 and v0.2 and neither the author of the PR nor the reviewers did realize.

Is there any discussion about why the old design where you pass a Handler when creating sockets was abandoned?

@carllerche
Copy link

I'll respond in more depth in a bit, but could you elaborate on "Just this morning we had a panic in Polkadot ". What was behavior (end result) that lead to a panic? I would expect that an early panic would be more obvious than implicitly having multiple reactors running on the background w/o any indication of this happening?

@carllerche
Copy link

It sounds like you want an explicit API where you must specify the runtime handle to use the socket. Otherwise, you you either end up w/ a runtime panic or you end up w/ unexpected multiple reactors running in multiple background threads.

Off the top of my head, one option would be to add fns on runtime::Handle to create the various resource types (sockets, timouts, ....) and require a feature flag to opt-in to the context based constructors (like TcpStream::connect).

So:

rt_handle.tcp_connect("....").await // -> TcpStream

rt_handle.delay_for(....).await

This would probably need to go via an RFC period in an issue.

@tomaka
Copy link
Member

tomaka commented Dec 12, 2019

What was behavior (end result) that lead to a panic?

We were called tokio0.2::spawn_blocking from within a Future that was polled using tokio v0.1. It panics because no tokio environment is detected.

@en
Copy link

en commented Dec 12, 2019

What was behavior (end result) that lead to a panic?

We were called tokio0.2::spawn_blocking from within a Future that was polled using tokio v0.1. It panics because no tokio environment is detected.

You may try this https://github.com/tokio-rs/tokio-compat

@tomaka
Copy link
Member

tomaka commented Dec 12, 2019

You may try this https://github.com/tokio-rs/tokio-compat

The problem is not so much how to make it work. The fix was done in 5 minutes.
The problem is that we would like to have some guarantee (preferably at compile time) that it is going to work.

@thomaseizinger
Copy link
Contributor

thomaseizinger commented Jan 2, 2020

You may try this tokio-rs/tokio-compat

The problem is not so much how to make it work. The fix was done in 5 minutes.
The problem is that we would like to have some guarantee (preferably at compile time) that it is going to work.

I totally understand that reasoning and personally, I'd also appreciate it if there would be compile-time guarantees for this.

It would still be nice to leave the choice up to the end-user on what to use. For example, we make use of a lot of libraries built on top of hyper which means we are already using tokio anyway.

It is a bit of a shame to now having async-std being forced upon us by using rust-libp2p, not just in regards of a heavier dependency tree but also with it implicitly spawning a background thread. (Don't get me wrong, it is an awesome project and if the decision is to stick with async-std, we will just have to swallow that pill I guess 😁 )

Without driving this issue too much off topic: are there any discussions in the community of how to handle this situation? Any shims that wrap both async-std and tokio and allow you to choose the backend through feature flags or something?

@tomaka
Copy link
Member

tomaka commented Jan 2, 2020

I'd be in favour of adding tokio equivalents of TcpConfig named for example TokioTcpConfig.

I think that the default TcpConfig should use something that "just works" (i.e. async-std here), but adding alternatives that are more optimized in favour of some trade-off (i.e. here, surprise factor if you're not using tokio) is totally fine.

@dignifiedquire
Copy link
Member

For what it is worth, I am in the other boat, and would like to avoid being forced to use tokio.

I agree that in theory it would be be great to allow for an easy switch between the two, but all experiments in that regard that I have seen so far looked quite painful to maintain, and quite a lot of overhead for the library authors.

@vorot93
Copy link
Contributor

vorot93 commented Mar 21, 2020

@tomaka just to let you know, async-std's executor and I/O runtime seem to be unmaintained since its author is out.
https://twitter.com/stjepang/status/1241028964765913092?s=21

@tomaka
Copy link
Member

tomaka commented Mar 23, 2020

@tomaka just to let you know, async-std's executor and I/O runtime seem to be unmaintained since its author is out.
https://twitter.com/stjepang/status/1241028964765913092?s=21

We're not using async-std's executor. That's the entire point of this whole decision: not being tied to a specific executor. If we used tokio's sockets we would be tied to tokio.

@tomaka
Copy link
Member

tomaka commented Mar 23, 2020

As far as I know, all that remains to do before closing this issue is make libp2p-mdns socket-agonostic, and then everyone will be happy with that additional bloating.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants