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

RFC: Tokio reform, take 2 #3

Merged
merged 13 commits into from Feb 6, 2018

Conversation

@aturon
Contributor

aturon commented Sep 27, 2017

Rendered

This RFC proposes to simplify and focus the Tokio project, in an attempt to make
it easier to learn and more productive to use. Specifically:

  • Add a default global event loop, eliminating the need for setting up and
    managing your own event loop in the vast majority of cases.

    • Moreover, remove the distinction between Handle and Remote by making
      Handle both Send and Sync and deprecating Remote. Thus, even working
      with custom event loops becomes simpler.

    • Allow swapping out this default event loop for those who want to exercise
      full control.

  • Decouple all task execution functionality from Tokio, instead providing it
    through a standard futures component.

    • When running tasks thread-locally (for non-Send futures),
      provide more fool-proof APIs that help avoid lost wakeups.

    • Eventually provide some default thread pools as well (out of scope for this
      RFC).

  • Similarly, decouple timer futures from Tokio, providing functionality instead
    through a new futures-timer crate.

  • Provide the above changes in a new tokio crate, which is a slimmed down
    version of today's tokio-core, and may eventually re-export the contents
    of tokio-io. The tokio-core crate is deprecated, but will remain available
    for backward compatibility. In the long run, most users should only need to
    depend on tokio to use the Tokio stack.

  • Focus documentation primarily on tokio, rather than on
    tokio-proto. Provide a much more extensive set of cookbook-style examples
    and general guidelines, as well as a more in-depth guide to working with
    futures.

Altogether, these changes, together with async/await, should go a long
distance toward making Tokio a newcomer-friendly library.

Rendered

@aturon aturon referenced this pull request Sep 27, 2017

Closed

RFC: Tokio reform #2

@aturon

This comment has been minimized.

Contributor

aturon commented Sep 27, 2017

I've made several updates to the RFC based on the feedback here, but due to a Github glitch, I need to put this in a new PR. In any case, here are the key changes:

  • The RFC now includes a fleshed-out API for customizing the default reactor, which can be used to prevent any automatic thread-spawning. This gives you complete control if you want it.

  • The timer APIs are moved out to a futures-timer crate, and by default spawn a dedicated timer thread (as per @carllerche's argument). We eventually plan to make this behavior customizable as well, but want to punt on that bit of design work for now.

  • The current_thread APIs are significantly tightened up, and in particular the panic-on-drop behavior has been removed. The main downside to this change is that, if you want to couple together a reactor and a single-threaded executor, you'll need to do this manually using FuturesUnordered. However, external crates (like tokio-core!) can provide this coupling for you.

@aturon aturon changed the title from Tokio reform, take 2 to RFC: Tokio reform, take 2 Sep 27, 2017

@carllerche

This comment has been minimized.

Member

carllerche commented Sep 27, 2017

Thanks for the update.

Re: SetDefault

The documentation states:

This function is intended to be used within applications at executor granularity; it is not intended as a general-purpose mechanism to eschew passing a Handle for library APIs.

Given that executor::Enter is used to statically define the context of an executor, I'm curious why SetDefault does not require a &Enter as an argument? This would ensure that this API is used at the executor level instead of ad-hoc to specify a reactor via thread-local.

In general, the expected usage patterns of SetDefault and how one hooks into arbitrary executors to call it seems undefined.

Re: futures-timer crate

I think the Timeout name should be reserved for an actual timeout operation. Aka, wrapping a future and erroring it if a set delay elapses.

What the RFC refers to as Timeout should probably be named Delay or Sleep.

Re: Reactor::turn

I think the API described would technically work to allow hooking in a current thread executor, but it would require an additional layer of indirection (similar to how FuturesUnordered is implemented).

I think the best option here is just going to be for Reactor to provide some sort of Wakeup handle that simply forces Reactor::turn to unblock (similar to how this works in Mio). This way, the current thread executor can build its own Notify implementation that calls Wakeup::wakeup (or whatever API) and tracks the id passed to notify however it wants.

This should avoid that extra layer of indirection.

// with Tokio, read and write components are distinct:
let (reader, writer) = conn.split();
CurrentThread.spawn(async {

This comment has been minimized.

@seanmonstar

seanmonstar Sep 27, 2017

Collaborator

Looks like this is still using the older CurrentThread.

#![feature(conservative_impl_trait)]
extern crate futures;
extern crate tokio_core;

This comment has been minimized.

@twmb

twmb Sep 27, 2017

For clarity, I think all examples based on tokio and not tokio-core should use extern crate tokio and use tokio::xyz below.

@stepancheg

This comment has been minimized.

stepancheg commented Sep 28, 2017

As I mentioned before, error type should be Never here:

        pub fn spawn<F>(&self, task: F)
            where F: Future<Item = (), Error = !> + 'static;

Because future is not meant to return an error, it must handle error itself and return success.

@seanmonstar

This comment has been minimized.

Collaborator

seanmonstar commented Sep 28, 2017

While it'd be neat to use ! here (and indeed, it's been discussed a few times in the context of futures that cannot error), it's still unstable: rust-lang/rust#35121. Maybe one day.

```rust
impl Timeout {
fn new(dur: Duration) -> Result<Timeout>

This comment has been minimized.

@sfackler

sfackler Sep 28, 2017

Can we defer the error here into the Future's return value? Since Result implements IntoFuture, it's somewhat common to accidentally do something like foo.select2(Timeout::new(timeout)), which compiles but immediately resolves rather than actually waiting on the timeout.

impl Stream for Interval { .. }
```
These functions will lazily initialize a dedicated timer thread. We will eventually

This comment has been minimized.

@sfackler

sfackler Sep 28, 2017

It seems a bit unfortunate we'd need to dedicate a thread to this rather than leaning on epoll's timeout field like is done now.

This comment has been minimized.

@carllerche

carllerche Sep 28, 2017

Member

@sfackler

See #2 (comment)

That said, the new design allows running timers backed by epoll_wait. It just isn't the default.

This comment has been minimized.

@tanriol

tanriol Sep 28, 2017

Will the current same-thread heap-based timer infrastructure be kept somewhere (possibly deprecated) or dropped/reimplemented on top of a timer thread? Will we need to write our own timer heap to have same-thread timers?

This comment has been minimized.

@sdroege

sdroege Sep 28, 2017

That's also my only remaining worry. While the situation is cleared up around the reactor and executor threads (and should cover my use cases), having the default timer implementation automatically spawn a thread still is a problem. Anything using timers would uncontrollably spawn a thread without having a way to override that right now.
See also @alexcrichton 's comment here (the part about timers).

Apart from that I have no further worries/objections with the new version of the design.

This comment has been minimized.

@carllerche

carllerche Sep 28, 2017

Member

@tanriol @sdroege Yes, it will be possible to setup the timer + i/o reactor + executor on a single thread. This is the point of the refactor. The default of a separate thread is only that, a default. See the comment I linked above for an explanation as to why it is a default.

This comment has been minimized.

@sdroege

sdroege Sep 28, 2017

It certainly makes sense as a default, yes. Thanks for confirming that this will be part of the refactoring of futures-timer, nothing else to add from my side in that case and I'm quite happy with the changes 👍

FWIW, my worry is less about having it as a separate thread (which indeed makes sense in the majority of situations, just like the reactor/executor use different threads by default) but having no control over the lifetime (and usage) of that thread.

### `tokio-io`
The `tokio-io` crate needs an overall API audit. Its APIs may eventually be
*re*exported within the `tokio` crate, but it also has value as a standalone

This comment has been minimized.

@sfackler

sfackler Sep 28, 2017

The *s here are messing up the markdown formatting downstream

impl Future for Timeout { .. }
impl Interval {

This comment has been minimized.

@bkchr

bkchr Sep 28, 2017

Can we add a function range(min: Duration, max: Duration) that does the same as in my pr tokio-rs/tokio-timer#33?
I use this interval in a project where I want to contact a server, but as we have multiple thousand devices, not all devices should connect at the same time. So, this range brings some randomness into the wait time for a new connection attempt.

This comment has been minimized.

@tailhook

tailhook Oct 12, 2017

While I also use random intervals all the time, it's too specialized. For example, your PR makes a default/uniform distribution for the interval, but I would say it should rather be normal/gaussian distribution to make interval more regular (or maybe there is some better random distribution for the specific use case of intervals?)

@RoelofSol

This comment has been minimized.

RoelofSol commented Sep 28, 2017

I'm still not sure about the global default handles. It's better than before but it's still black-magic-behind-the-scene-fuckery anti pattern, but in practice it would work.

The biggest problem i have right now is that some hypothetical new project will simply leave out the signature with the handle variant:

// set up a fully-customized listener fn from_listener(listener: std::net::TcpListener, handle: &Handle) -> io::Result<TcpListener>;

The workaround would be to "just set the default" if i understand the RFC correctly.

IMO it would be more elegant from a newcomer's perspective to see "Simple" or "Auto" newtypes variants that use the default handle.

That way , somebody who is exploring the inner workings would immediately be told by the code: " To make it easier for beginners we provide a global handle. But if you want to create something similar, you should take a handle as an argument and wrap it with a "Simple" newtype like this".

Anyways.

Awesome project and keep up the good work.

@arthurprs

This comment has been minimized.

arthurprs commented Sep 28, 2017

Can we make spawn take any future?

pub fn spawn<F, S, E>(&self, task: F) where F: Future<Item = S, Error = E> + 'static;

We're doing something similar with crossbeam defer().

It's a bit annoying to massage the futures in order to make spawn happy.

@jkozlowski

This comment has been minimized.

jkozlowski commented Sep 28, 2017

not sure how useful this is, but I was working on the idea of global event loops here (modelled on seastar), before I ran out of steam: https://github.com/jkozlowski/tokio-smp. It indeed greatly simplifies the code. I was definitely thinking of epoll driven timers, but also smp message passing between cores...

I am definitely liking the focus on simplification +1

) -> Self;
fn tokio_reactor(&mut self) -> &mut reactor::Reactor;
fn task_runner(&mut self) -> &mut current_thread::TaskRunner;

This comment has been minimized.

@alexcrichton

alexcrichton Sep 28, 2017

Contributor

I think this may be an old remnant from a previous iteration, and I think that additionally we won't be able to provide Handle::spawner below with the RFC as-is. I think it may be ok to remove these, though? I figured that the Core::tokio_reactor and Handle::tokio_handle methods are pretty important, but should we also figure out how to retain methods like Core::task_runner, Handle::spawner, and Core::from_tokio?

- Uses of `Remote`/`Handle` that don't involve spawning threads can use the new
`Handle`.
- Local task spawning should migrate to use the `current_thread` module.

This comment has been minimized.

@alexcrichton

alexcrichton Sep 28, 2017

Contributor

I think current_thread here should be thread and/or Spawner

This API should be used in functions like `block_until` that enter executors,
i.e. that block until some future completes (or other event
occurs). Consequently, nested uses of `TaskRunner`, or usage of
`current_thread::block_until` within a `TaskRunner` or thread pool, will panic,

This comment has been minimized.

@alexcrichton

alexcrichton Sep 28, 2017

Contributor

I think this sentence wants to get updated to say that nested usage of block_until or block_on_all will panic (no more TaskRunner)

`run` and `remote` methods and the `Executor` implementation:
```rust
impl<'a> From<&'a Reactor> for NotifyHandle { /* ... */ }

This comment has been minimized.

@alexcrichton

alexcrichton Sep 28, 2017

Contributor

Was it intended to leave this in with the addition of the wakeup method? (seems fine to me to remove it for now to be as conservative as possible)

@aturon

This comment has been minimized.

Contributor

aturon commented Sep 28, 2017

I've pushed updates:

  • I've addressed @carllerche's concern about the use of Notify, by instead providing Handle::wakeup as a direct method.
  • I've renamed Timeout to Delay.
  • I've added a Enter::make_permanent method, which is needed when you want to use an external thread pool as an executor (by leveraging worker initialization hooks on the thread pool).
  • The SetDefault type is removed in favor of Handle::make_default_for, which takes a mutable reference to an Enter, thus tying reactor defaults to executor initialization. To make this possible, Enter provides an on_exit method for registering exit hooks.

I think the main remaining issue is that the RFC, as it stands, does not provide any way to override the default timer thread. I'm trying to punt on that for the moment, in part because @carllerche believes we probably want to tweak the overall API. So I'd like to take the futures-timer part of this RFC as declaring an intent to provide a crate with the described functionality, plus customization, but with full details blocked on a separate RFC.

@aturon aturon requested a review from carllerche Sep 28, 2017

@3n-mb

This comment has been minimized.

3n-mb commented Dec 22, 2017

@seanmonstar , I thought that talking rx here, in tokio's rfc, is appropriate, as it suggests the future direction. Yet, after couple of months, may be you are right. May be, we should give up on tokio's adapting to our needs, and make reactive-rust thingy to have both rx and reactive streams. And, may be in the future, rx+mio and reactive streams + mio will become a go-to place for those shopping in tokio 😄

@eyoung @mathstuf would you care to make reactive-rust group to host experiments, actual code, rfc's ?

@carllerche

This comment has been minimized.

Member

carllerche commented Dec 22, 2017

@3n-mb Again, there is no reason for Tokio to add any "rx" concepts. futures-rs provides all the building blocks to build your own higher level abstractions that are compatible with the ecosystem.

If you want to see rx concepts happen, the quickest strategy is to implement it yourself.

@3n-mb

This comment has been minimized.

3n-mb commented Dec 22, 2017

@seanmonstar & @carllerche this thread explains why we talk rx here:

First, someone is looking for a standardized reactive lib, i.e. lib done like it is done in other languages, facilitating language-neutral structuring of an app.

Then, invariably, there will be an answer like:

You can use tokio purely without its networking stuff.

But non-standard approach in tokio will bite you hard.

@carllerche

This comment has been minimized.

Member

carllerche commented Dec 22, 2017

@3n-mb once again, Tokio is only for I/O. futures-rs provides the asynchronous building blocks. Everything rx related can be built on top of futures-rs.

I would appreciate it if the rx related discussion could move to the forum or an issue on the futures-rs repo.

@3n-mb

This comment has been minimized.

3n-mb commented Dec 22, 2017

@carllerche can you put in docs that

Tokio is only for I/O.

Thank you.

@3n-mb

This comment has been minimized.

3n-mb commented Dec 22, 2017

@eyoung @mathstuf
I've registered organization reactive-rust as a place for rx things. Please, join. Everyone else is also welcomed, for non io-centric useful abstractions.

@flosse

This comment has been minimized.

flosse commented Jan 28, 2018

I'd like to reference my question: tokio-rs/tokio-proto#202

@carllerche

This comment has been minimized.

Member

carllerche commented Feb 6, 2018

I'm going to merge this.

Most of the work has already happened in the tokio crate.

@carllerche carllerche merged commit 039898f into tokio-rs:master Feb 6, 2018

@nayato

This comment has been minimized.

nayato commented Feb 7, 2018

is there a rough ETA on the work settling? Will this be blocked on futures 0.2?

@nayato

This comment has been minimized.

nayato commented Feb 7, 2018

@carllerche

This comment has been minimized.

Member

carllerche commented Feb 7, 2018

No, the changes described in this RFC are not blocked on futures 0.2. The tokio reform changes will be pushed to crates.io very soon.

There are a set of breaking changes in Tokio that require coordinating version releases. Specifically, breaking changes in mio, tokio-io, bytes, and iovec. Those will be coordinated with futures 0.2.

@asomers

This comment has been minimized.

asomers commented Feb 13, 2018

Is there any documentation for migrating from tokio-core to tokio? In particular,

  • How can I execute a future to completion and return its value? With tokio-core I could use Core::run, but that's gone. In its place there seems to be only current_thread::spawn, but it can't return a value.
  • The borrow checker is complaining where it never did before, and it's not obvious why.

@FreeFull FreeFull referenced this pull request Feb 13, 2018

Open

Tokio reform #165

@cetra3 cetra3 referenced this pull request Feb 14, 2018

Closed

Is Handle Send + Sync? #139

@GoldenBadger

This comment has been minimized.

GoldenBadger commented Feb 20, 2018

@asomers

This comment has been minimized.

asomers commented Feb 24, 2018

@GoldenBadger carllerche had a different idea: current_thread::block_on_all.

tokio-rs/tokio#143

@carllerche

This comment has been minimized.

Member

carllerche commented Feb 26, 2018

There will also be CurrentThread::block_on which will only block on the supplied future and not all futures spawned on the executor.

@joshlf

This comment has been minimized.

joshlf commented Mar 15, 2018

Has there been any discussion of a way to spawn non-'static futures when you're in a block_on_all call? I've often found myself wanting to store variables in the local function scope and construct futures which can access those variables and will run with block_on_all. This would technically be safe, except that if those futures wanted to spawn futures, it doesn't work because spawn_task still requires its argument to be 'static. I think this kind of scoping would make it easier to provide static guarantees about resource cleanup and would make those sorts of async applications that much easier to write.

@cramertj

This comment has been minimized.

cramertj commented Mar 15, 2018

@joshlf You could use FuturesUnordered or spawn the types, or you could use a custom wrapper which joins them using a JoinHandle via an API similar to crossbeam's.

@joshlf

This comment has been minimized.

joshlf commented Mar 16, 2018

Hmm interesting thought; I'll try that. Thanks! Is there a reason that Executor::spawn requires a static lifetime rather than the same lifetime as the Executor itself? In other words, is it possible for a future that you spawn on an Executor to outlive that Executor?

@cramertj

This comment has been minimized.

cramertj commented Mar 16, 2018

@joshlf The executor is a trait object, and its lifetime is unknown.

@carllerche

This comment has been minimized.

Member

carllerche commented Mar 16, 2018

@joshlf currently, the thread pool included with the futures crate does this (futures outlive the executor).

@carllerche

This comment has been minimized.

Member

carllerche commented Mar 16, 2018

@joshlf

This comment has been minimized.

joshlf commented Mar 16, 2018

OK, thanks for the clarification.

@sdroege

This comment has been minimized.

sdroege commented Mar 29, 2018

As one common concern with the default runtime was that there's no control anymore (just some magic thing happening in the background, which it is actually not in the current implementation: you explicitly create a runtime), and implementing a fully single-threaded solution becomes impossible, I've some example here (that was written as a testcase for a performance problem) for future reference. It was not entirely obvious to me from just reading the documentation and required reading the tokio code.

This basically explicitly creates something like the default runtime with a configurable number of threads and runs a future on it, this does the same but completely single-threaded (Reactor and Executor on the same thread, like in old tokio-core).

Note: there's no shutdown code, no error handling, no spawning of additional futures in here but that can easily be added. Take a look at the tokio Runtime code for some guidance.

@carllerche

This comment has been minimized.

Member

carllerche commented Mar 29, 2018

I would recommend discontinuing discussion here and instead moving to issues on http://github.com/tokio-rs/tokio.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment