Consider removing spawning from futures::task::Context

[Ralith]

The Context struct currently conflates two independent concerns: allowing futures to arrange to be notified via Wakers, and allowing new tasks to be spawned. I don't believe these belong together. Wakeup handling is useful to practically all leaf futures that aren't serviced by kernel mechanisms (i.e. that aren't leaf I/O futures), so it makes sense to ensure these facilities are passed down to every leaf. By contrast, very few futures require the ability to spawn tasks, and those that do are typically in application code (for example, the accept loop of a server) where an executor handle can be easily made available. In the rare case where library code genuinely needs to spawn new tasks, this can be easily accomplished by explicitly taking an executor handle, or by returning an impl Stream<impl Future> whose elements can be spawned in whatever manner is appropriate to the application.

The specifics of spawning a task can also vary considerably between executors in ways the generic interface exposed by Context cannot support. For example, applications which require non-Send futures or which can't perform dynamic allocation cannot make use of Context-based spawning at all. This not only leads to awkward vestigal API surface, but also presents a subtle compatibility hazard: code using an executor that does not support spawning via Context will compile fine when combined with libraries that assume one, but fail at runtime when spawning is attempted. By contrast, if the ecosystem standardizes on returning streams of futures, spawning (and guarantees such as Sendability of futures to be spawned) naturally becomes explicit.

cc @carllerche, @Nemo157

[carllerche]

I am also strongly against including an executor with the Task context.

The argument for including it is that it always allows spawning in all future related context. However, this is not true. Instead, adding a context spawner adds yet another way to spawn, adding to the confusion.

In my experience, I have rarely needed to be able to spawn from library code. The only case in which it comes up is very high level APIs (like hyper's high level API). All other libraries never spawn. Instead, they return objects that allow the caller to control how they get spawned. For example h2 does not require spawning, instead it returns a Connection object that the user is expected to drive (usually by spawning).

In all of my cases where spawning from a library is required, the spawner passed to the context object is insufficient. This is because it is a) a trait object and b) forces Send on spawned tasks, and c) would limit the flexibility of the library.

a) and b) are related. Being a trait object prevents using typed executors (executors that are designed to only execute tasks of a specific type, not T: Future<...>). Also, requiring Send forces the library to only work in Send contexts even if the library is built to handle both Send and !Send.

Instead, tower-h2 takes an E: Executor<Background<T>> where Background is the task that needs to be spawned and T represents a user provided action. In this case, if T: !Send, then the executor provided must be a "current thread" executor. By representing the executor this way, tower-h2 does not force Send.

c) If the library is built to only be able to spawn with a context object, this prevents spawning from drop fns. Being able to spawn from a drop fn is necessary to be able to run async cleanup code given that drop cannot block.

Because of these limitations, I do not believe the spawn argument to context will be used much at all with Tokio, instead Tokio's executor system will be the preferred method. I do not believe it to be a good idea to include a spawn argument in the type provided by std when it is not globally useful.

[MajorBreakfast]

@carllerche Thanks for this feedback. I don't have an opinion on this yet, but I find it immensely important that we discuss this properly. Here are some of related thoughts:

• Make the context generic: Rust loves to enforce everything through the type system. A generic context would make it possible to make clear what things are needed to run a future, e.g. Future<Spawn + tokio::Runtime, Output = ()>. This would make things very explicit and possibly very optimiser friendly.
• Investigate whether a task_local! macro, like @Ekleog proposed in Build task_local! data from a Future rust-lang/futures-rs#1187 is interesting as a building block

[Ralith]

@MajorBreakfast both of those options seem to presuppose that it's worth passing an executor handle implicitly to every single future, which I don't think should be assumed, particularly given the rarity of spawning in practice.

[MajorBreakfast]

@Ralith Focus not so much on spawning, but on the event loop example I gave. For instance Tokio's futures require that there's an event loop around. Without a generic context the way you find out if there's an event loop around is by seeing it fail at runtime. With a generic context, there would be a trait bound that enforces that the context implements the event loop functionality. Same story for spawning. Spawn would be just one of many traits the generic context could implement.

[carllerche]

IMO that would be a good route to explore regardless of spawning. The needs of Tokio are very different than embedded, etc...

[Thomasdezeeuw]

I'm also in favour of removing spawn from the future's context. But that only leaves waking in the context, which, to mee atleast, is really the only required part of the context to run (poll) the future. Maybe rather then using a generic context, futures should only require a Waker as argument. To me that is only thing that is essentiel to (effectively) run/poll a future. All other functionally can be provided by the runtime system type.

Another argument against a generic context is actually the lack of being generic. Futures have proofed to be a difficult subject to learn and understand. Adding more complexity, via a generic context, might result in people just saying "just use Tokio's context/types" (or any other runtime). This would have the adverse effect on the genericity (is that even a word?) of Future types build by the ecosystem.

[aturon]

I agree with @Thomasdezeeuw.

I think we should remove the concept of Context entirely, and simply pass poll a LocalWaker, making wakeups the only fundamental requirement that applies to all futures. While I had initially hoped that we could have a useful notion of "default executor", it seems too fraught, and as many people have pointed out, it's somewhat rare to require spawning in library code anyway.

[cramertj]

+1-- the huge advantage here for me is that we get to remove Context and just pass &LocalWaker into futures. IMO this drastically simplifies the API and makes things easier to understand.

Also, @carllerche made the excellent point to me on Discord that libraries which return an impl Future type for the user to run, though less ergonomic, allow for a great deal more flexibility in terms of how the user schedules the object (including allowing non-'static lifetimes, as well as choosing whether to make the type Send or not).

Combined with the fact that end applications will usually make use of a thread-local spawner anyways for convenience (since they're available even outside of async contexts) I think Context isn't pulling its weight. I'll work on drafting a PR to remove it so we can push ahead, but please speak up if you think Context should stay! (There will, of course, be time to re-review this decision when the final std::future RFC comes up for review, but ideally we'd have consensus on this and land the desired API in std before that.)

[Ekleog]

FWIW, I use Context to spawn a future in https://github.com/Ekleog/erlust/blob/master/erlust/src/spawn.rs#L14, to mirror tokio::spawn. Removing the Context would mean I would have to either:

• just return the Future and let the user spawn it themselves (which isn't as user-friendly IMO, especially given this future wrapper must be run directly as a task -- and in particular nesting two such future wrappers would most likely wreak havoc), or
• depend on tokio to be able to use tokio::spawn

Both of which options sound… unhelpful.

All that to say, I think the ability to spawn from a future does make sense, and a blanket removal would be a step backwards. Then, @MajorBreakfast's solution of having some contexts implement the SpawningContext trait and some not implementing it would solve the issue of having contexts unable to spawn, while leaving open the opportunity for me to require a SpawningContext.

[Ralith]

I'm not sure what that code is for (things that want to mirror tokio::spawn are generally executors), but it could perfectly well just take a reference to the spawner you want to wrap explicitly instead of relying on it being bundled into the context.

[Ekleog]

I should have given more context to this code indeed.

So basically, here there are two kinds of Tasks: normal tasks, and actor tasks. Actor tasks have additional setup/teardown, which is done via LocalChannelUpdater. Hence my not wanting to just return a future and let the user decide how to spawn it: the user could then chain other stuff outside of the LocalChannelUpdater or put two LocalChannelUpdaters one inside the other, which would do Bad Things.

However, I don't want/need to write an executor, I can just run the system on any executor that supports spawning tasks. Hence my not willing to just use tokio::spawn, as that'd force the user to use tokio.

The solution of requiring the user to explicitly pass a spawner is a solution indeed, but makes it way less comfortable to the user, as they would have to actually pass said spawner all around their stack, which would infect just about all the code… or just use (an equivalent to) tokio::executor::DefaultExecutor::current(), which would just move things that could be made generic over all executors into just the tokio executor. And given I don't see any reasonable use case for anything else than the default executor… I'd rather just hardcode this and have an easy-to-use API, but without depending on tokio.

BTW, the code I'm referencing here that makes the difference between actor tasks and regular tasks is more or less a manual implementation of the task_local! macro @MajorBreakfast mentioned in #56 (comment)

[Nemo157]

they would have to actually pass said spawner all around their stack, which would infect just about all the code

having some contexts implement the SpawningContext trait and some not implementing it would solve the issue of having contexts unable to spawn, while leaving open the opportunity for me to require a SpawningContext

These result in having to change pretty much the same sets of code, either you need to be generic over a Spawn and pass an instance of that everywhere, or be generic over a SpawningContext and have that implicitly passed. (One major question with the latter is how it would interact with async fn, there's currently no way to add these generics to the returned future. The former is much easier to integrate.)

If you're writing an actor system that has task_local!s for actors, couldn't the spawner just be one of those task locals? You init the actor system with an initial spawner for the executor it's running on, then have that implicitly passed around to all the actors (maybe with a way to contextual change it for sub-systems that want to use an alternative spawner).

[Thomasdezeeuw]

* this future wrapper _must_ be run directly as a task

@Ekleog I'm not sure what you mean by this. However reading your next comment I think your concern is basically running the destructor when the future returned Poll::Ready, correct? Would a wrapping future work? Something like this:

/// The public future that is returned.
pub struct Future {
    /// Once `ActualFuture` returns `Poll::Ready` this will be taken (`Option::take`) and
    /// `ActualFuture` will be dropped so it can run its destructor.
    inner: Option<ActualFuture>,
}

This, I think, would solve running your future as a task. As for running futures for the user of a library, I think most of the ecosystem will return futures for the user to run themselves. But I could be wrong here.

[Ralith]

@Thomasdezeeuw I think that's what he's already doing.

@Ekleog

Hence my not wanting to just return a future and let the user decide how to spawn it: the user could then chain other stuff outside of the LocalChannelUpdater or put two LocalChannelUpdaters one inside the other, which would do Bad Things.

This seems like a problem that can be solved with documentation and diagnostics without incurring all the drawbacks of the Context API. For example, if you have task-locals, you can use them to assert that LocalChannelUpdater::poll never nests, and provide a helpful panic message.

Perhaps I still don't fully understand what you're doing, but it also seems perfectly legitimate for a user to want to chain things (for example, diagnostics, or other futures that might not interact with your system at all) after your cleanup work.

The solution of requiring the user to explicitly pass a spawner is a solution indeed, but makes it way less comfortable to the user, as they would have to actually pass said spawner all around their stack, which would infect just about all the code

It's unusual that "just about all the code" would be spawning tasks. For every application or service I've seen, spawning takes place in one or two places and everything else is composition. Is that normal for your usecase?

Ultimately, this strikes me as another case of magically-propagated information being superficially convenient, but not actually letting you do any things that aren't otherwise possible, and the drawbacks are considerable.

[seanmonstar]

It's unusual that "just about all the code" would be spawning tasks.

However, as I explore switching hyper to use 0.3, it turns out to be that I now need to propagate a Context argument through "just about all the code", and not because all the code needs a waker. Only because a couple of places do, and so it's just become noisy ceremony.

I realize this is slightly off-topic to the original issue, but as it was suggested to replace the Context with just a LocalWaker, I'd want to re-consider having to pass any argument at all.

[Ralith]

@MajorBreakfast
In the course of recent discussion in the tokio-rs/dev gitter, I've come to feel that generic futures are an inappropriate solution to expressing dependency on an I/O event loop. Wakers are the fundamental primitive of futures execution: all communication, either with the outside world or between futures, is or can be mediated through them. An I/O future that relies on a specific event loop does not rely on the mechanism by which it's awoken (i.e. polled), only the mechanism (such as mio) by which its waker is associated with the external I/O event. A tokio TCP future can run on any executor; the fact that it relies on a reactor is only obscure because that dependency is realized through thread-locals.

Making futures generic on executor wouldn't help, because those futures are genuinely executor-independent: they are only unusual in that their wakers are invoked externally, which is something wakers already explicitly support in the general case. If we wanted to make the reactor-future dependency explicit, it would be as simple as requiring that the reactor be passed in when the I/O future is constructed, which is a decision that can be made by the implementer of the I/O future and reactor. The futures API itself is unaffected.

I think this also further strengthens the case that wakers are special and are uniquely entitled to being propagated universally, whereas e.g. spawning is not.

[Ekleog]

@Nemo157

If you're writing an actor system that has task_local!s for actors, couldn't the spawner just be one of those task locals?

That's a possibility indeed, and likely what I'll do if that change passes. Unfortunately, an actor task requires locking into a global hashmap for creation, which potentially has the cost of blocking the executor, so I didn't want to force all tasks to be actors in this system. There are most likely ways around it, that said.

@Ralith As @seanmonstar points out, even if you need a spawner at only one place, you need to propagate it through all the functions that lead to that place, which will soon become a problem.

For example, if you have task-locals, you can use them to assert that LocalChannelUpdater::poll never nests, and provide a helpful panic message.

Honestly, I think if I had task-locals, I wouldn't even be asking for the Spawner to be implicitly passed as argument. However, as task-locals are not provided, this means I need to implement them myself, which is actually the reason why I don't want the Spawner to be pushed out of futures: I can't see a way to sanely implement task-locals without controlling at least a bit the spawning process.

As for flexibility, nothing prevents a spawn function from taking the task_local / context-provided (considering the two as basically equivalent) Spawner, and a spawn_on function from taking the Spawner as an argument, so I don't think flexibility should be that much of an issue here.

All that to say: were the proposed change coupled with the addition of task_local!, I don't think I'd have any issue with it. Without it, I think usability decreases too much.

[Ralith]

@Ekleog I think @seanmonstar's point was more about the effort of passing a context argument through a stack of hand-written futures code. He's arguing for the removal of any sort of context entirely. It sounds like you have only one place that needs a spawner, and it's called directly by the library user, so the impact is very small in comparison. The initial discussion in this issue covers why a narrow built-in spawn function isn't a satisfactory solution at some length.

[carllerche]

@Ekleog IMO implementing task-locals by decorating spawn is a hack. The executor itself should provide enough hooks to do so.

[Ekleog]

@Ralith I somehow agree that a narrow spawn function isn't satisfactory, but not having any way to have a default spawner is even less satisfactory. Because the library users will have to propagate the spawner through all their code until all places where they actually call the spawn function, which isn't something I'd wish for as a library author. Allowing to override the default spawner does make sense (and appears to be the core of the discussion of the first few posts to me, after reading it), but having a default spawner is IMO highly useful too from an end-user-simplicity point of view.

And the end-user-simplicity point of view appears to not have been discussed until my first comment, hence my making it :)

Also, it sounds like my understanding of @seanmonstar's point differs from yours, even though now I'm doubting I'm understanding correctly, so let's let them clarify. :)

@carllerche I totally agree with you. However, task_local! appears to have been dropped from futures-0.3, so I guess it's not coming back? And using an executor-specific task_local! is a no-go, as IMO libraries should be as executor-agnostic as possible. Then, it's still possible to ask the user to do the task_local! handling themselves (to alleviate the cost of passing the default spawner through all their stack), but that still makes for more boilerplate than necessary, so I'd lean towards putting the spawner's task_local! inside erlust… which would mean requiring an executor-independent task_local!.

If task_local! comes back in an executor-independent fashion, then it's not an issue IMO to remove the spawner from the context :) because then there is most likely no longer a need for strictly controlling spawning.

[cramertj]

@Ekleog

not having any way to have a default spawner is even less satisfactory. Because the library users will have to propagate the spawner through all their code until all places where they actually call the spawn function, which isn't something I'd wish for as a library author.

The alternative is having a standard crate like log which exposes a thread-local spawner that can be set by executors and used by libraries. The downside to this approach is that you need some way to ensure that the executors folks want to use are all using this lib.

[Ralith]

You could also have an external crate that implements a common interface for task locals in general. Executors that don't support it directly could be bridged into by wrapping futures or the executors themselves, as convenience dictates, all mediated by a thread-local.

Not all executors would want to implement task-locals or spawning; for example, a non-allocating executor driving a fixed set of tasks probably wouldn't use either, which is fine. Since it's an external crate, you don't end up with any vestigal std interfaces needing to be stubbed out with panic.

edit: Task-local discussion should probably go here: #7

[Ekleog]

@cramertj The problem with this solution is exactly embodied by the log crate: it's not actually used by every crate that wants to log, and some users would prefer using slog. Arguably task-locals are a much simpler interface than a log interface, but… if it's so, then it could just as well be baked in futures-0.3, this way everyone would actually use it :)

@Ralith I'm not convinced by the external crate solution for task-locals, as it requires a hook into the spawning process that's currently not present (as mentioned in #7's top post).

Good point for moving the discussions about task-locals to #7, though IMO resolution of this issue should be blocked on resolution of #7, for the reasons stated above :)

[tikue]

I don't think the current situation is any different from the situation without spawn-via-context, because neither solution provides hooks into the spawn process. Presumably solving spawn hooks can happen regardless of where spawning happens from?

[Ekleog]

Well, the current situation allows wrapping tokio::spawn in an executor-independent way, which for sure isn't optimal but is way better than just being unable to do anything user-friendly. :)

Then, if I understand rust-lang/rust#54339 (comment) correctly, the change to remove spawning from the Context has already been approved, so it's no longer useful discussing the usefulness of making the change only after #7 is solved.

[boomshroom]

I'd just like to interject that thread local storage isn't always an option, namely in no_std environments. Whatever solution we come up with has to be able to work for no_std executors. This was the whole reason the Context struct was made in the first place.

[cramertj]

@boomshroom futures 0.1 supported no_std executors by allowing them to provide custom non-thread-local storage for these objects. That is still an option now.