Skip to content
This repository was archived by the owner on Oct 30, 2019. It is now read-only.
This repository was archived by the owner on Oct 30, 2019. It is now read-only.

Consider removing spawning from futures::task::Context #56

@Ralith

Description

@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

Activity

carllerche

carllerche commented on Sep 4, 2018

@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

MajorBreakfast commented on Sep 4, 2018

@MajorBreakfast
Contributor

@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

Ralith commented on Sep 4, 2018

@Ralith
Author

@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

MajorBreakfast commented on Sep 4, 2018

@MajorBreakfast
Contributor

@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

carllerche commented on Sep 4, 2018

@carllerche

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

Thomasdezeeuw

Thomasdezeeuw commented on Sep 6, 2018

@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

aturon commented on Sep 17, 2018

@aturon
Contributor

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

cramertj commented on Sep 17, 2018

@cramertj
Collaborator

+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

Ekleog commented on Sep 18, 2018

@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

Ralith commented on Sep 18, 2018

@Ralith
Author

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

Ekleog commented on Sep 18, 2018

@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

Nemo157 commented on Sep 18, 2018

@Nemo157
Member

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).

12 remaining items

Ekleog

Ekleog commented on Sep 20, 2018

@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

tikue commented on Sep 20, 2018

@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

Ekleog commented on Sep 21, 2018

@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.

added a commit that references this issue on Sep 23, 2018
boomshroom

boomshroom commented on Sep 27, 2018

@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

cramertj commented on Oct 1, 2018

@cramertj
Collaborator

@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.

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    WG async/awaitIssues relevant to the async subgroup

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @carllerche@seanmonstar@Nemo157@Ralith@MajorBreakfast

        Issue actions

          Consider removing spawning from futures::task::Context · Issue #56 · rustasync/team