_NIOConcurrency: Rename completeWithAsync(_:) to completeWithTask(_:) and return task

[simonjbeaumont]

Return Task from EventLoopPromise.completeWithAsync(_:) wrapper.

Motivation:

Bridging an async function into NIO is done by calling completeWithAsync(_ body:) on an EventLoopPromise. This spins up a Task and awaits the result of the async closure that was passed in and uses the result to fulfil the promise with the value of the function, if it was successful, or with the error, if the function threw.

In some cases we may want to cancel this operation from the outside.

Modifications:

This patch adds a discardable return value to EventLoopPromise.completeWithAsync(_:) which is the Task it creates.

Result:

Users of EventLoopPromise.completeWithAsync(_:) are now able to explicitly cancel the Task that is being used to fulfil the promise.

[simonjbeaumont]

//cc @glbrntt

[glbrntt]

Soundness should be fixed by #1913

[Lukasa]

Seems reasonable to me.

[weissi]

@Lukasa doesn't this leak that we do create a new Task? CC @ktoso

[ktoso]

If you mean "leaks implementation detail" then kind of yeah, but it does not cause a "leak leak".

Let's think if this leaking of impl detail is actually "impl detail" or "necessary detail"...

There's basically two ways to cancel a task:

• 1. the parent is cancelled, and thus we are as well
• 2. the task's handle (Task<...>) is cancelled explicitly (a task in a group is similar, group.cancelAll() is the same story), and an async let task getting cancelled because a throw happened is also basically <the task of the async let>.cancel())

In that sense... Since this API is intended to be called from a "probably not async context" (maybe it is a sync function but actually running inside of a Task, but in any case we potentially cannot await in this scope), so we can't really rely on 1), so indeed exposing the Task so that it may be cancelled is the only other option.

Perhaps a different name here would be better though? let task = promise.completeWithTask { <clearly this is a task> } and task.cancel() reads pretty logical, it also explains what the returned value there is, wdyt?

The alternative, without offering such API, is to force people to: let t = Task { promise.completeWithAsync/Task { ... } } which then they might cancel, so I don't think it's worth making the life of developers harder here, might as well just return the task we created.

[Lukasa]

I'm with @ktoso: while it absolutely leaks an implementation detail, this is a detail that is unavoidably necessary for participating correctly in structured concurrency.

[Lukasa]

I would be open to renaming to completeWithTask though.

[Lukasa]

And concretely the reason this change was proposed was for the grpc-swift project, which will want something like this. The fact that they need it suggests that it's likely to be generally useful.

[simonjbeaumont]

Ah... I see I broke the api breakage pipeline. I didn't realise this underscore module was intended to be API stable at this point. Should I leave the original one in (that returns void) and add a completely separate one with a fixit?

[Lukasa]

The underscore module is not API stable, but the api breakage script doesn’t know that. We can merge over it.

[ktoso]

and "yay!" for the rename while at it :)