tokio::task::JoinSet should support adding tokio::task::JoinHandles

[abc-mikey]

** the problem ** It should be easy to spawn tasks and have them added to a JoinSet being managed in another green-thread. If JoinSets do not accept JoinHandles which are easy to pass between threads then the options are to put the JoinSet behind some kind of locking or to pass Futures to the thread where the JoinSet resides to be spawned there, which requires Boxing and Pinning the Future.

** fixing the problem ** JoinSet is simply spawning a task and inserting JoinHandle into it's internal state. I would like it to have a function that takes a JoinHandle and simply does the insert.

** the alternatives ** I can reimplement my own version of JoinSet that allows for JoinHandles, but so will anyone else wanting to use them in this way.

** context ** Allowing passing of JoinHandles means that the JoinSet can be used to do whatever has to happen whenever a task completes or panics in its own task without any performance cost in starting a new task. Alternatively using locking or passing Futures to that thread via channels will introduce a slight delay before starting each new task.

[hawkw]

I think adding a public API method like the current JoinSet::insert would be a useful addition to the JoinSet API!

I wasn't sure why we went with the current API design where futures have to be spawned by the JoinSet::spawn{_whatever} methods, rather than just inserting their JoinHandles, so I took a look at the PR that originally introduced JoinSet (https://github.com/tokio-rs/tokio/pull/4335) to see if I could find the motivation for this design. As far as I can tell, it wasn't discussed in this PR. My guess is that the intention was to preserve the ability to change the internal implementation of JoinSet to one that requires control of how the tasks are spawned and couldn't accept a JoinHandle from a task spawned elsewhere, without breaking the API.

@Darksonn, was that your intention when designing the JoinSet API? And, do you think there's still value in preserving that optionality? If that's important, I think we could consider introducing a public JoinSet::insert-like method as #[cfg(tokio_unstable)], at least in the short term.

[Darksonn]

So, currently the JoinSet allocates for each task in addition to the task's own allocation. However, I had originally wanted to inline that extra allocation into the task's own allocation to avoid that. I didn't do it at the time because it was complicated, but I also did not add methods to add a JoinHandle to preserve the possibility of doing this optimization in the future.

[hawkw]

Hmm, I think inlining the JoinSet list into the per-task allocation is definitely a potentially very valuable optimization, especially given that this is something that can only be implemented in a type provided by tokio --- a third-party library can't easily implement a JoinSet-like type that requires no additional allocation. Preserving the optionality to implement something like that might be worth keeping the current API, especially if it was something we were actively planning to add...

[PaulOlteanu]

Is this optimization something we want to look into? (I could be interested in taking a look). If I'm understanding this correctly, something like #6132 would not be possible if we implement this optimization right - since the handles in that iterator would have been spawned outside the context of the JoinSet?

[Darksonn]

Yes ... but I would prefer if this change was made by someone who is already familiar with the src/runtime/task module. It currently does not have support for adding extra fields to the trailer depending on how the task is spawned, and I don't want to add them to tasks not spawned using this mechanism. Adding support for that gets complicated.

[hawkw]

I might take a crack at implementing this optimization in the next week or so if I have some free time.

[bryanlarsen]

For clarity, is this a no to this request?

[Darksonn]

Yes.