Consider passing the Task that is driving the future to Future::poll and renaming the function

[rjuse]

Currently Future::poll seems to be expected to call task::park which then fetches the current task from TLS and panics if there is no task in TLS.

This results in an unintuitive API (it's not clear at first glance that poll()'s expected interface/implementation is related to tasks) and a potential run-time failure that could be checked at compile time.

So my suggestion is to instead pass the task driving the Future explicitly to Future::poll as an additional function argument, either as a Task reference, a closure calling task::park() (if that's enough), or a similar mechanism, instead of storing it in the TLS variable CURRENT_TASK.

Also, "poll" is a confusing name, since it creates the expectation that it is a function that anyone can call to get the value of the future if it has already completed, but it is in fact an "internal" function that drives future execution instead and currently even panics if called outside a task.

Something like "drive", "execute", "run", "run_next_step" or similar would be a better name.

[aturon]

FWIW, we actually started with a design that passed an explicit &mut Task argument. That ended up posing problems for things like implementing the direct Read trait, which also wants access to a task. Mediating this through thread-local storage made it possible to support standard I/O APIs while implicitly accessing the task, and also greatly improved ergonomics.

I do agree that thread-local storage has downsides, in terms of being less explicit and in our case opening you up to runtime errors. However, I think that once you understand the basics of the library, this kind of mistake is very rare and will very reliably product panics (so shouldn't be hard to debug).

Regarding the naming, I can see what you mean but none of the suggested names particularly evoke "tasks" for me in a way that the current name doesn't.

[rjuse]

I think implementing the Read trait this way might not be the optimal design (and also might be broken currently if it's called outside of a Future's poll function, although I suppose that's easily fixed by doing nothing if there is no current task).

A possible alternative that seems more sound is to implement, if desired, the Read trait in a way that is purely nonblocking and thus does not access the current task.

End users would normally use APIs that return futures (and that thus aren't Read or similar traits), while the internal implementation of those APIs can work with internal functions that take a task argument instead of the Read trait.

[aturon]

The key downside of that alternative is that the work of managing callbacks to the task is no longer automatically managed by the core I/O abstractions, as it is today. In other words, with today's setup, there's no way to create a "lost wakeup" when working with primitive I/O, because the interaction with the task is handled automatically. Moving to this model was a big part of what changed when merging futures-mio into tokio-core, and I think it was a big improvement.

[aturon]

I think a more plausible alternative would be to take an explicit task argument everywhere, including in read and write. That would have the benefit of making the task requirement more explicit, and making a panic impossible, while retaining the guarantees of the current implementation. That might enable some simplifications in today's executor story, but I'm not sure.

The downsides would include:

• Loss of ability to use standard I/O traits and hence loss of some interoperation
• Loss in ergonomics

[rjuse]

But aren't end users supposed to normally use APIs that return futures, which already implicitly handle everything correctly?

This is how other future-based systems work; for instance, in .NET, to read data, a TCP server would call Stream.ReadAsync which returns a future.

The Read trait can just not be implemented at all and then if the only APIs available either return futures, or require passing the task explicitly, it's also impossible to have lost wakeups.

Being able to use standard traits with implicit wakeups seems to be unsound in general and thus have problems in nontrivial code: for example, if someone sends the I/O stream to another thread (perhaps due to some automatic parallelization framework, or because they are using an Actor-like model like Servo), then the wakeup will not be registered to the proper task, since the other thread can't possibly know the right task unless the task is explicitly passed as well.

Of course it's possible to document the pitfalls and accept the potential for run-time issues in case of misuse, but it doesn't seem ideal.

[aturon]

But aren't end users supposed to normally use APIs that return futures, which already implicitly handle everything correctly?

It depends a little bit on which "end users" you have in mind. The current I/O integration does not expose the bottom-level reading/writing facilities as futures. That's because we don't want to impose ownership transfer of buffers.

Let's take reading for an example. You'll need a buffer to read into. If you represent this as a future of the read being complete, then you have to hand over an owned buffer for the whole time you're waiting for a read to occur. That in turn means you may have to have a large number of such buffers allocated and in flight, if you have a large number of concurrent reads.

By contrast, the Read trait is designed to allow you to pass in a buffer slice at the moment a read is attempted, without transferring ownership. In the futures world, you wait on readiness for a read (i.e. that there appear to be bytes available), and only at that point do you attempt the read. If in fact there aren't bytes ready, you haven't given up any buffer ownership, and are automatically scheduled to be notified for readiness again.

All of the above considerations really matter when you think about something like a proxy server, which ideally can serve any number of clients with just a single buffer. See the example proxy server where we put this into action.

[carllerche]

Being able to use standard traits with implicit wakeups seems to be unsound in general and thus have problems in nontrivial code: for example, if someone sends the I/O stream to another thread (perhaps due to some automatic parallelization framework, or because they are using an Actor-like model like Servo), then the wakeup will not be registered to the proper task, since the other thread can't possibly know the right task unless the task is explicitly passed as well.

In the case you are describing, sending the I/O stream to another task is a supported use case. When an I/O stream is sent to another task, the original task will receive a spurious wakeup, but that is fine. The new task will receive the I/O stream, attempt to poll from it and the I/O stream will now be registered with the new task, which will be woken up once the stream is ready.

If you send a future to a thread that is not managed by a future task, then currently there is a panic, however I think a better alternative would be returning io::Error::new(io::ErrorKind::WouldBlock, ...). In this case, the receiving thread will either know what to do with the error or not, but this is already the case if it received a mio::TcpStream or any type in general that can return "would block" /cc @aturon

[rjuse]

What I'm talking about is complex code that implements Future::poll() by spawning several scoped threads or otherwise sends work to be executed synchronously on a thread pool (perhaps implicitly due to using an auto-parallelization library like rayon) and then accesses a Tokio's TcpStream in those separate threads.

In this case, these read/write calls should register wakeups on the task driving the Future::poll, but since they are in separate threads, the TLS CURRENT_TASK variable will not be properly set, and either the function would panic or the wakeups would be silently missed.

Explicitly passing the task would obviously either make the code work correctly or cause a compilation error instead of having those issues.

Propagating TLS variables can fix some cases of this, but I don't think it can work in general and there might be issues even without threading (the Future::poll implementation for future A driven by task A somehow creating a closure that ends up executed inside the Future::poll implementation for future B driven by task B, and having the wakeups from that closure being registered for task B while instead they should have been registered for task A, due to the task not having being captured in the closure).

[rjuse]

I wrote this to try to summarize and make the arguments of the thread clearer; it doesn't add much to the previous discussion in the thread, but might perhaps reframe it in a more orderly and easily followed form.

I think the first design question is whether libraries and programs that aren't directly interfacing with the OS should implement the Future trait themselves or should instead only or mainly create futures using combinators like map/and_then/etc or using the "oneshot" facility.

The SOCKS5 example actually uses both coding styles, implementing the handshake via future-based combinators (starting from the "read_exact" / "write_all" Future-based I/O APIs in Tokio), but the data transfer with a custom Future::poll implementation using Read/Write traits on TcpStream.

If users are not supposed to implement Future themselves, like in other designs such as .NET and JavaScript promises, then only future-returning APIs are used to create futures via combinators, or the user does the "wakeup" itself using oneshot, and thus the task-handling concerns are not present.

BTW, it is possible to have a Future-returning I/O API that doesn't keep buffers in flight, for example by returning a Future<ReadableHandle> and having a read method that takes the ReadableHandle and a buffer and actually reads the data in an instantaneous way once the future is completed (type ReadableHandle = () or usize is a simplified case of this).

If users are instead supposed to implement Future themselves (which might be needed in Rust to ensure optimal performance can be achieved, which is not a goal of other GC-based future designs), then it becomes useful to provide a low-level API that is non-blocking but not future-based, as well as a facility to register interest in unparking a task or both combined in a single API, for use in implementations of Future::poll().

The current design of futures-rs/tokio-io provides such a facility by implementing the Read/Write traits in a "special" way that consists of a normal non-blocking implementation that also "magically" fetches the current task from the TLS and potentially parks it and has it unparked when the stream becomes ready again.

This means that wakeups cannot be accidentally missed in simple code, but as far as I can tell can be missed in complex code that passes the I/O stream to other threads or creates, for example, closures that do I/O and are then potentially called by the Future::poll() of other unrelated futures/tasks.

The possible change explored in this issue is, assuming that letting users implement Future directly this way is useful, to make the wakeup registration explicit, and thus add a task argument to Future::poll() and either change the Read/Write implementations on tokio-io's TcpStream and similar to be purely non-blocking implementations and have separate ways to register the task to be unparked, or have read/write methods that mimic the Read/Write trait and take a task argument (the latter avoids potential missed wakeups, but is also less flexible and might result in unnecessary wakeups).

If Read/Write is not implemented, there would be a loss of interoperation with other code using standard I/O traits, but such code might assume that they are implemented in a blocking way as they would on a file or blocking socket, and might not work anyway on a non-blocking implementation as the one in a futures framework without full "green threads" must be (regardless of whether it interacts with task parking).

[carllerche]

Just a quick note:

If Read/Write is not implemented, there would be a loss of interoperation with other code using standard I/O traits, but such code might assume that they are implemented in a blocking way as they would on a file or blocking socket, and might not work anyway on a non-blocking implementation as the one in a futures framework without full "green threads" must be (regardless of whether it interacts with task parking).

I would not say this is correct. I've found that I have been able more often than not to reuse existing code that assumes std::Read / std::Write. For example, the extension fns on the traits themselves "just work", also the various SSL libs that implemented SSL decoration for any Read + Write types worked out of the box.

[carllerche]

To clarify, implementing Read / Write to handle would block situations is not a new idea introduced by futures/tokio, this is how non-blocking IO works today and is how it will continue to work today. If a user receives a would-block error, they are responsible for figuring out how to get notified when readiness changes. In the case of futures / tokio, it would be by using a task, in the case of mio, it would be by registering interest w/ Poll.

[rjuse]

I just realized that there is a third option for read/write that might be the best: continue to implement the traditional Read/Write traits as they are implemented now in Tokio, but instead of implementing them directly on TcpStream, implement them on a struct containing a reference to the TcpStream and to the task.

In other words, an implementation of Future::poll() would call stream.read_for_task(task) with the task reference they got passed as an additional argument and get a value that implements Read and automatically registers wakeups on task, or potentially would use a closure-based API with stream.with_read_for_task(task, |read| ...).

This would remove the need to use TLS and make task passing explicit, while otherwise keeping the current code unchanged.

This API also allows the implementation to defer some of the wakeup registration work to the destructor of the struct implementing Read or at the end of the closure, in case that allows better performance.