SE-0300: Continuations for interfacing async tasks with synchronous code

ktoso · 2021-01-17T07:56:38.658Z

Chris_Lattner3:

Why does this leak a task? Is this a logic error like a leak or is this a memory safety violation?

This is best illustrated by an example, consider the following snippet:

await hello() // I'm in *some Task*, I keep it around while I wait

func hello() async { await with*Continuation { c in /* don't do anything */ }

results in the task awaiting on hello() to "hang forever". So it is not that the withContinuation leaks anything directly, but rather that since the waiter never gets resumed, it remains retained (and waiting (!)) forever.

This is a serious issue (equally bad as deadlocking I guess), however it is not really a memory safety issue per se.

Chris_Lattner3:

These types are only used when interoperating with legacy completion handler APIs that should eventually be replaced with more SwiftConcurrency-native centric APIs over time.

Yeah though that is a narrow view on this API. It is not only for objective-C APIs that will be replaced with async APIs, but also for interacting with "some C API that I don't own that works via callbacks, and it will never change".

Many examples of such APIs show up in the server ecosystem, and include database drivers or other low-level APIs which we don't control or even kernel APIs, where we might want to express our shims over them as async functions. It would be very annoying if for very low level libraries such as these, we were limited from using async functions because of the overheads caused by the checked API. Note that even with the checked: false shape you propose it would cause additional ARC traffic as it has to power the deinit warning being triggered.

I did not dive deep into this specific topic, however e.g. NIO's wrappers around epoll and kqueue would be nice to express with async functions rather than a callback how it does today (func whenReady(strategy: SelectorStrategy, _ body: (SelectorEvent<R>) throws -> Void) throws -> Void), but it's certainly another example of an low-level API that would not want to sacrifice any performance on for a integration with swift's async system.

Though epoll wrappers I guess would end up as an async functions that blocks anyway, since hitting epoll_wait so the specific example might not be the best... but you get the idea -- in those very low level APIs, where we might want to use an async function, and definitely do not want any additional overhead, I don't see a different way other than offering two types.

Ben_Cohen · 2021-01-17T16:57:51.020Z

benrimmington:

There could be an async Result.init , so the returning: and throwing: argument labels would make less sense.

There can be fewer APIs (i.e. a single Unsafe*Continuation type, with a single resume method).

You’re still describing outcomes here, not benefits. Why is it better to use Result to cut back the API surface from 2 methods to one? Why is using Result with Never better than a throwing and non-throwing variant?

Max_Desiatov · 2021-01-17T17:32:59.070Z

Ben_Cohen:

Why is it better to use Result to cut back the API surface from 2 methods to one? Why is using Result with Never better than a throwing and non-throwing variant?

For me personally this would satisfy the principle of least surprise. Result is pretty much a standard right now when dealing with callback-based asynchronous code. It's much easier to just pass a Result value as is from a callback to resume instead of destructuring it manually into success and failure cases.

Similarly, Combine doesn't have separate Publisher and ThrowingPublisher protocols. I would expect that integration of async functions with Combine's publishers makes a lot of sense, having separate throwing and non-throwing continuation types makes it inconsistent.

Ben_Cohen · 2021-01-17T20:18:18.095Z

Max_Desiatov:

Result is pretty much a standard right now when dealing with callback-based asynchronous code.

Result is standard for callbacks because there is no mechanism for them to support throwing instead. So instead there need to be different design patterns to take its place. There is no ThrowingPublisher for similar reasons, not stylistic ones. But async/await provides this mechanism.

A Result-based approach was considered back when Swift’s error handling was originally designed. I’d very much discourage people pushing Result as an alternative to throwing with this proposal if this boils down to “if I designed Swift from scratch, I’d use Result instead of Swift’s error handling mechanism everywhere”. Such preferences won’t be considered to “fit well with the feel and direction of Swift” by the core team.

Nothing here is ruling out stylistic preferences for returning Result from async functions in cases where it suits better, just as you can from synchronous functions today. But arguments that this proposal shouldn’t take advantage of async functions being able to throw because Result is fundamentally a better approach than throwing probably won’t get very far.

Max_Desiatov · 2021-01-17T20:40:07.279Z

I understand that it's not directly related to this exact proposals, but this is also inconsistent with Task.Handle<Success, Failure: Error> in the structured concurrency pitch. Would it make sense for that to be updated to Task.Handle<T> and Task.ThrowingHandle<T> instead?

I was originally (before this proposal review thread was created) evaluating continuations as a part of the structured concurrency pitch, and the inconsistency there between two separate continuation types and only one task handle type was glaring.

Ben_Cohen:

I’d very much discourage people pushing Result as an alternative to throwing with this proposal

What's the reasoning then for inclusion of getResult() in addition to simple get() in task handles for structured concurrency?

extension Task {
  struct Handle<Success, Failure: Error>: Equatable, Hashable {
    /// Retrieve the result produced the task, if is the normal return value, or
    /// throws the error that completed the task with a thrown error.
    func get() async throws -> Success
    
    /// Retrieve the result produced by the task as a `Result` instance.
    func getResult() async -> Result<Success, Failure>
  }
}

I understand that the answer could be that "continuations and structured concurrency are separate proposals now and should be discussed separately". But I only want to understand where exactly the use of Result “fits well with the feel and direction of Swift” and where it doesn't. What's the distinction here between continuations and task handles that justifies the inclusion of getResult() and making the error type explicit in the latter, but not in the former?

benrimmington · 2021-01-18T09:10:27.005Z

Ben_Cohen:

Why is it better to use Result to cut back the API surface from 2 methods to one? Why is using Result with Never better than a throwing and non-throwing variant?

By having fewer API variants, I find them easier to learn and use. I was also trying to limit top-level Unsafe* and withUnsafe* APIs, for better auto-completion of the existing pointer APIs.

Ben_Cohen:

I’d very much discourage people pushing Result as an alternative to throwing …

I now understand your concern, but that wasn't my intention. My earlier example uses try result.get() immediately.

benrimmington:

Are the with*Continuation methods misnamed? The continuation is allowed to escape the given closure, unlike other with* methods in the standard library.

Could this be improved by adding a static suspend method to each continuation type?

try await UnsafeThrowingContinuation.suspend { continuation in
  /* ... */
  continuation.resume(returning: value)
}

JJJ · 2021-01-18T20:29:42.239Z

benrimmington:

Are the with*Continuation methods misnamed? The continuation is allowed to escape the given closure, unlike other with* methods in the standard library.

I'm not too happy about "await with" either :-)

How about "await continuation"? Or "await continuation block"?

You could even rename the closure argument "continuationHandle", or even "promise" as this has the same semantics as the closure argument to Combine.Future()...

try await unsafeContinuation { promise in
  /* ... */
  promise(.success(value))
}

??

Joe_Groff · 2021-01-21T20:19:26.721Z

graskind:

Are checked continuation failures due to multiple resumes assertions (debug config only), preconditions (debug and release), or "hard" checks (effective when built with -Ounchecked )? In short, do "checked" continuations devolve into "unsafe" continuations in release and/or -Ounchecked builds? (For the record, given the choice, I would want them to be preconditions - corrupted process state is still corrupt in a release build!)

Along the same line, do warnings about checked continuations being lost appear in release builds?

The checks (both traps and warnings) should happen at all optimization levels. I can make this explicit in the proposal.

Jon_Shier:

I don't see anything in the proposal about APIs which provide completion handlers and the queue they run on. Customizable completion queues have been the recommended API design for a while now, it would be a shame if they weren't supported. (If we hadn't gotten rid of DispatchQueue.current , that would seem to be an ideal choice here.)

The queue from which resume is invoked is irrelevant, since it only transitions the task out of the suspended state; tasks already know what executor they're associated with, and will be scheduled on whatever queue/runloop/thread/other scheduling mechanism their current executor uses. If an API takes a queue argument on which to run its completion handler, it's probably best to pass the queue that the code dispatching the completion handler will run on, if you know it. In cases when we know a task's work can be safely enqueued on a specific dispatch, to allow the queue hops to be minimized, we could perhaps provide some API on Task to get that queue for the current task, and have an unsafeResumeImmediately variant of API on *Continuation that immediately resumes execution of the task on the current thread, relying on the assumption that the code invoking resume was already executed by the correct queue. This would be a very sharp knife, though, and using it incorrectly could lead to really subtle problems that'd be tough to diagnose, so I think it'd be good to take a wait-and-see approach to see if queue-hopping becomes a bottleneck in practice. Despite the API recommendations, nearly all of the completion-handler-based APIs in Apple's frameworks do not take a queue argument, and there's not a lot of consistency in how completion handlers are invoked, so very few Apple APIs would be able to take advantage of such an optimization today. And as more code adopts Swift's native tasks and async functions, the Swift compiler and runtime's own optimizations for avoiding expensive queue hops will hopefully reduce overall overhead in time.

Chris_Lattner3:

What is the expected overhead of CheckedContinuation, and is it possible to avoid that overhead without duplicating the API?

As currently implemented, UnsafeContinuation is a plain pointer to the task structured to be resumed, whereas CheckedContinuation allocates a class instance to hold the task pointer, so compared to UnsafeContinuation, it'll have ARC operations applied to it when copied around, and will use atomic operations to "take" the task pointer from the instance the first time it's resumed so that the double-resume protection is thread safe. When we have move-only types, we should be able to make a safe
Continuation type that has to be resume-d in order to dispose of it; before then, I don't see an obvious way to avoid the overhead in the language today. However, even with move-only types, since a lot of the
value of this API is interop with existing C and ObjC APIs, I wonder whether a move-only continuation type would be very practical to use, or just force the use of unsafe escape hatches down a layer.

Lantua:

Either the resume function must only be called exactly-once on each execution path the operation may take (including any error handling paths), or else

the resume function must be called exactly at the end of the operation function's execution.

Why are these separate cases? The latter seems to be included in the former.

I can reword the proposal to make this clearer. The distinction that we're trying to make is between resume happening after the with*Continuation block has finished executing and the task has suspended, and resume happening while the with*Continuation block is still executing. An example of the latter would be:

await withUnsafeContinuation { c in
  if condition {
    // Suspend the task and resume it later
    doSomethingAsynchronously(completion: { c.resume(returning: ()) })
  } else {
    // Resume the task immediately
    c.resume(returning: ())

    // According to rule 2, we can't do anything after `resume` if we run
    // it immediately
  }
}

I'll amend the text to make this clearer.

Max_Desiatov:

I think there was a question previously in the pitch thread that I don't remember being addressed, asking why UnsafeContinuation<T> and UnsafeThrowingContinuation<T> (same for the checked variants) are separate types instead of introducing a single UnsafeContinuation<Result, Failure: Error> type?

This separation of types and making the error untyped is inconsistent with Task.Handle<Success, Failure: Error> in the structured concurrency pitch, and the already present Result<Success, Failure: Error> type.

I also don't see this addressed in the proposal itself. Is there a specific reason for this?

The error propagation from with*Continuation is either-or; if you specified a more specific Error type, it would have no type system impact on what the caller sees when withUnsafeThrowingContinuation throws. Task.Handle's error parameterization seems a bit suspect to me for this reason too; in practice it can only ever be Never or Error.

Chris_Lattner3:

Why does this leak a task? Is this a logic error like a leak or is this a memory safety violation? If this is a memory safety problem, then this should probably also be an unsafe API, even if it is slightly safer. If it is memory safe but "just" a logic error, then I don't think the rationale above is super strong. We have lots of logic errors that happen from API misuses, and we don't dance around that. For example, "Sequence" in a generic context is an unchecked single iteration sort of beast and is often misused, we don't encode fear into the name.

It's a leak, and potentially a deadlock if other work relies on the abandoned task finishing, but it's not a memory safety violation. If the task is never resumed, it'll sit around forever holding onto whatever resources it was using when it suspended. My bigger concern about making leaking a task trap is the lack of determinism in where that trap happens; one could miss a trap because naive -Onone ARC put off destroying the checked continuation wrapper until the process ends for other reasons, and then end up shipping a -O build that cuts the lifetime shorter and exposes the trap.

Jon_Shier · 2021-01-21T20:45:07.151Z

Joe_Groff:

The queue from which resume is invoked is irrelevant, since it only transitions the task out of the suspended state; tasks already know what executor they're associated with, and will be scheduled on whatever queue/runloop/thread/other scheduling mechanism their current executor uses. If an API takes a queue argument on which to run its completion handler, it's probably best to pass the queue that the code dispatching the completion handler will run on, if you know it. In cases when we know a task's work can be safely enqueued on a specific dispatch, to allow the queue hops to be minimized, we could perhaps provide some API on Task to get that queue for the current task, and have an unsafeResumeImmediately variant of API on *Continuation that immediately resumes execution of the task on the current thread, relying on the assumption that the code invoking resume was already executed by the correct queue. This would be a very sharp knife, though, and using it incorrectly could lead to really subtle problems that'd be tough to diagnose, so I think it'd be good to take a wait-and-see approach to see if queue-hopping becomes a bottleneck in practice. Despite the API recommendations, nearly all of the completion-handler-based APIs in Apple's frameworks do not take a queue argument, and there's not a lot of consistency in how completion handlers are invoked, so very few Apple APIs would be able to take advantage of such an optimization today. And as more code adopts Swift's native tasks and async functions, the Swift compiler and runtime's own optimizations for avoiding expensive queue hops will hopefully reduce overall overhead in time.

Based on other replies up thread, especially Ben_Cohen's, the queue on which resume is invoked is anything but irrelevant, as it determines the queue on which the resumed code is executed. Or is the phrase "absent anything else like actors being involved" doing the hard work there, because there's really no way to execute async code without being in a context that would guarantee execution resumes on the original queue? So is this real behavior (which would be an issue with this proposal), or is it a behavior that won't exist in the fully concurrency story? That is, to use the current snapshot as an example, where does the code execution resume in the example below?

runAsyncAndBlock {
    // On runAsyncAndBlock's backing queue.
   let string = await makeNetworkCall() // Wrapper which resumes a continuation on URLSession's arbitrary completion queue.
    print(string) // What queue is this on? runAsyncAndBlock's backing queue, or URLSession's completion queue?
}

And despite Apple's frameworks not usually following the practice, largely due to age or the friction of passing DispatchQueue's in Obj-C, it has certainly been the recommendation by Apple engineers on social media and in WWDC labs, as well as the community at large, to pass queues for completion handlers. So when people bring up best practice, we don't just mean the practices visible in Apple's SDKs but those the Swift community have been using over the last several years.

Joe_Groff · 2021-01-21T20:48:54.569Z

Jon_Shier:

Based on other replies up thread, especially Ben_Cohen's, the queue on which resume is invoked is anything but irrelevant, as it determines the queue on which the resumed code is executed.

resume does not immediately resume the task, it only makes it ready to be resumed again. Its executor/actor will resume the task in a context appropriate for its current state. I'll make this clearer in the proposal text. In your example, runAsyncAndBlock creates a task that takes over the main thread; after makeNetworkCall suspends the task, it ought to resume on the main thread once its continuation is resumed.

Jon_Shier · 2021-01-21T20:52:45.355Z

Ah yes, that does clear it up, thanks. With that being the case, queue hopping becomes an optimization problem I'm happy to leave for later.

michelf · 2021-01-21T22:37:06.663Z

Joe_Groff:

await withUnsafeContinuation { c in
  if condition {
    // Suspend the task and resume it later
    doSomethingAsynchronously(completion: { c.resume(returning: ()) })
  } else {
    // Resume the task immediately
    c.resume(returning: ())

    // According to rule 2, we can't do anything after `resume` if we run
    // it immediately
  }
}

The second comment in this example really bugs me. Does "can't do anything" imply no defer blocks? No ARC-triggered deinit? What happens if you do something like print("hi") after the c.resume line?

Joe_Groff · 2021-01-22T02:45:56.133Z

michelf:

The second comment in this example really bugs me. Does "can't do anything" imply no defer blocks? No ARC-triggered deinit ? What happens if you do something like print("hi") after the c.resume line?

After asking around, it sounds like this second restriction might not be needed. Certainly the current implementation shouldn't depend on it. I'll remove this.

Lantua · 2021-01-22T03:34:17.429Z

The first invariance:

Either the resume function must only be called exactly-once after the operation function passed into withUnsafeContinuation has finished executing, on every execution path through the program,

sounds tricky to comply. Even if I do:

withUnsafeContinuation { operation in
  DispatchQueue.global().async {
    operation.resume(...)
  }

  // Some clean up
}

There's no guarantee that DispatchQueue will be executed after operation has ended. Even if we don't have anything after DispatchQueue.async, it may still be executed while we're unrolling the stack, however unlikely it is.

Joe_Groff · 2021-01-22T17:18:11.340Z

I don't think this restriction is really necessary, and I asked the other authors of the proposal and we can't recall why that restriction was there. I'll remove it.

Lantua · 2021-01-22T17:29:52.672Z

Does this imply that continuation is not run on any executor, or at the very least, different one from the caller of withContinuation?

Joe_Groff · 2021-01-22T17:31:16.689Z

resume only transitions the task back to a schedulable state, and then returns control back to its caller. The task will resume execution when its executor from the time it was suspended reschedules it.

Lantua · 2021-01-24T19:03:12.236Z

So we could say that resume enqueues the next partial task and immediately returns. I'm all for simple rules, so removing the restriction is a welcomed addition.

What about withUnsafeContinuation itself? Does it enqueue operation on a different (unspecified) executor somewhere and immediately suspend the task, or does it execute the operation and suspend the task once operation completes? I think it's part of the semantic that should be documented. At the very least, we can decide whether to recommend not to perform long-running tasks inside continuation (though it should be done using Task.runDetached anyway).

Joe_Groff · 2021-01-24T20:50:43.824Z

The withUnsafeContinuation operation runs immediately in the about-to-be-suspended task's current context.

Ben_Cohen · 2021-02-02T04:50:42.848Z

Review Conclusion

Thanks to everyone who participated in this review. Given a number of clarifications of behavior emerged during discussion with the proposal authors, the core team has decided to run a second review of the clarified proposal before a final review decision.