Proposal: Convention for which types must be used on a Task

[carllerche]

Currently, it is somewhat unclear how to use the various future types with regards to tasks. poll must be called on a task, but besides that, what functions are safe to call vs. not is pretty unclear. Worse, some functions which must be called on tasks may not always fail when called not on a task. These functions may "randomly" panic depending on how concurrent operations complete.

Ideally, the type system could be used to enforce at compile time correct usage of future types, however I don't know how possible this is. Instead, I am proposing to clarify the usage of futures with regards to tasks using a convention:

All future types must be used on a task unless it is named Sync…

First, please don't focus too much on the prefix Sync*, I am just using it for now to make the proposal. Basically, the proposal is to avoid the ambiguity by making it a rule that all future types must be used on a task. This also means that all of the functions / constructors / etc… could have a debug_assert! that validates that they are called on a task. This should help find bugs quicker.

Future types can be made Sync by calling a sync() combinator.

Then, the question is how to use the various future types (Future, Stream, Sink, …) while not on a task. I propose that all of these traits include a sync() fn (again, a work in progress name) that wraps the type and allows it to be used off task. For example:

let f = my_future.sync(); // -> SyncFuture<MyFuture>

let result = f.wait(); // Blocks the thread, waiting for the future to complete
let result = f.poll(); // Returns the result if it has been completed
let value = f.unwrap(); // Equivalent to f.wait().unwrap();

The same could be done for Stream and Sink (though SyncSink is an odd name…)

APIs should be structured to discourage obtaining futures off of tasks

This would involve restructuring the APIs to setup users for success. For example, off the top of my head, Tokio's current API is something like this:

let reactor = reactor::Core::new().unwrap();
let connect = TcpSocket::connect(&addr, &reactor.handle());

let socket = reactor.run(connect).unwrap();

// Invalid...
socket.write(b"Hello world").unwrap();

The problem here is that this API is setup to use future types off of the task. You have to know that this is not the right way to do it…

Instead, one option could be to get rid of the Core::handle() function, and instead do:

let reactor = reactor::Core::new().unwrap();

reactor.run(move |handle| {
	let connect = TcpSocket::connect(&addr, &handle);
	connect.and_then(|socket| io::write_all(socket, b"Hello world"))
});

---

* I am using the prefix Sync because future types are async, and the opposite of that is sync.

[alexcrichton]

I feel like the default is likely "needs to be used on a task" because so many things transitively call task::park, but I could definitely imagine a convention being useful here.

[carllerche]

Right, I think this is basically what the proposal states. Part of the issue is that Tokio (and other) APIs don't clearly state this, and some examples are kind of fuzzy. The Sync* proposal is mostly for a conventional way to wrap a type for use off task.

To elaborate, using Sink off task cannot just be:

my_sink.send(foo).wait();

The start_send fn will probably need to call task::park() at times. For example, you did it here and it just "happens" to work in this case, but could easily fail in others.

[carllerche]

That being said... always using a sync() fn to wrap the type will mean collisions for types like Stream + Sink.

[carllerche]

Adding a quick thought before I forget.

@aturon mentioned that in his mind, all functions are safe to call off task unless they include Async* as part of the return value. Those functions include Future::poll, Sink::start_send, Sink::poll_complete etc...

However, today, there are functions that don't return Async* but require being run on the task. Most notably, all the tokio-core types that implement io::Read and io::Write. One solution could be to migrate to using AsyncRead and AsyncWrite traits instead.

Another issue that I just thought of is that I have often written functions like:

fn foo(&mut self) {
    if my_sink.poll_complete().is_ready() {
        // do some prep work
    }
}

In cases like this, it is not apparent by the foo function signature that foo is not safe to call outside of a task. The problem with using an opt-out convention (all functions are safe to call outside of a task unless they return Async*) is that it requires the author of the function to know about the convention and not accidentally forget to bubble up the Async. In the case above, I guess that the function should have been:

fn foo(&mut self) -> Async<()> {
    if my_sink.poll_complete().is_ready() {
        // do some prep work
    }

   Async::Ready(())
}

which is a bit silly and then the caller of the function would have to figure out what to actually do with the Async return value and do they guard against it ever being Async::NotReady?

The original proposal in this thread has the advantage of being "opt-in", so you take an extra step to ensure that a function is safe to call off task which will (hopefully) be less error prone.

/cc @aturon

[liranringel]

Perhaps we should consider passing a Task as an argument to every method that requires it?
For instance:
f.poll(get_current_task!());
That way, if there is no task in current thread, it will panic, always, at the same place.
It will also remove some of the "magic" in futures.

[carllerche]

@liran-ringel For reference, this has been discussed here #129

[seanmonstar]

What if parking a task didn't panic? Or in other words, park() returned an Option, so implementations can choose to not panic, and just return NotReady if there was no task when needed.

[aturon]

Obviated with the explicit Context argument