Reviews are an important part of the Swift evolution process. All review feedback should be either on this forum thread or, if you would like to keep your feedback private, directly to the review manager (via email or direct message in the Swift forums).
What goes into a review of a proposal?
The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift.
When reviewing a proposal, here are some questions to consider:
What is your evaluation of the proposal?
Is the problem being addressed significant enough to warrant a change to Swift?
Does this proposal fit well with the feel and direction of Swift?
If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
I'm strongly in favor of the functionality the proposal describes; there's no question in my mind that it fills a critical gap in the async task API.
I do have some questions on the specifics, though:
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?
Regarding the concerns around asserting on lost (never-resumed) continuations, I would note that SwiftNIO does assert for never-fulfilled EventLoopPromises; I'd like to better understand whether the considerations that went into that implementation would potentially raise meaningful counterarguments for the purposes of this proposal (while the implementations in question are obviously very different, NIO's EventLoopPromise does often serve a similar purpose to the continuations proposed here).
Unquestionably.
Very much so - while I sometimes lament some of the historical decisions that were made for compatibility reasons (coughObjective-C interop semanticscough), active support for integrating alternate (or legacy, or foreign) patterns and designs is one of Swift's great strengths. (Now, if only SwiftUI would follow the language's example! )
The API proposed here is an obvious evolution of promise fulfillment (such as that provided by SwiftNIO, as I also referenced above) and other asynchronous "wait for arbitrary event" designs. To put it simply: I like it .
Somewhere between a quick reading and an in-depth study.
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.)
DispatchQueue.current, thanks in part to queue hierarchies and a few other considerations, was pretty much never a safe API to use in the first place; while there is a concrete concept of the current thread, there really never was such a concept for a dispatch queue, at least not in a way that means what you'd want it to mean. The most significant issue is that there will almost always be more than one queue for which "is this the current queue" is true. (And worse, "workarounds" for this typically manifest as a huge proliferation of queues, which are not as cheap in practice as is commonly believed. Unfortunately, NSLock really is faster than a serial DispatchQueue.)
Right. My point was just that a way to get the current context and pass it as a DispatchQueue would be the ideal solution here, since that seems to be what async usually expects. I'm guessing that instead, the continuation will bring automatically jump back to the originating context, but I'm not sure, hence the question. I just wonder about the efficiency of that solution.
One could similarly make an argument that UnsafeContinuation shouldn't be exposed at all, since the Checked form can always be used instead. We think that being able to avoid the cost of checking when interacting with performance-sensitive APIs is valuable, once users have validated that their interfaces to those APIs are correct.
What is the expected overhead of CheckedContinuation, and is it possible to avoid that overhead without duplicating the API?
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.
There's one thing that's unclear to me: what does resume do?. Does it
Execute the next partial task, then returns once the partial task suspends/completes, or does it
Simply queue the next partial task to the global executor and returns immediately?
I don't think we need completion handler and the Task instance for this proposal just yet. These seem to be together in structured concurrency. And I don't think it'd change the shape of the APIs here.
+1 though I don’t see why we need these to be top level functions. Could we have them nested in some sort of empty enum for namespace and discoverability?
I would also love to see NIO use this API and hopefully provide feedback before we accept it.
Initially they were actually nested in Task, and we changed that some time ago... So either could be fine really... but it also does not really work with tasks per-se -- the fact that it is an async function rather means that it must be called from some existing task, and we resume it at some point.
So yeah, either top level or nested in Task, though I don't remember the specific reason we ended up prefering the top-level one rather the ones defined in Task
We have discussed and pretty much designed this API with such integrations in mind
NIO can simply do:
extension EventLoopFuture {
func get() async throws -> Value {
return await try withUnsafeThrowingContinuation { cont in
self.whenComplete { result in
switch result {
case .success(let value):
cont.resume(returning: value)
case .failure(let error):
cont.resume(throwing: error)
}
}
}
}
}
to provide an initial integration with async await and the existing ELF types. This can be done before NIO requires a Swift version that requires async await which is excellent; Once it decides to make a breaking change and adopt async in their APIs it could do so, but until then such extension can be provided by a compat module.
The unsafe API is boils down to a raw pointer to the task object.
Since some continuation heavy APIs are likely to try to optimize away any un-necessary atomic operations we feel it reasonable to provide an API that does not do this (both in terms of storage and the operation itself).
That's because async/await turns the entire pattern "on its head" kind of.
Previously when calling an API you would pass also "call me (the completion handler) on this queue":
call(on: someQueue) { done in ... }
This is turned around 180' with async/await, as the "I need to run on a specific queue" instead is expressed by simply awaiting in a context that ensures you'll hop back to it -- i.e. an actor. So such API becomes:
actor class X {
func x() async {
let done = await call() // we resume on the X's context, no need to pass in where we want to resume
}
So in that sense, the pattern of "passing in" where one wants to be resumed is replaced by calling async functions from a context which ensures the resumption happens on that context (an actor). This is core to the async/await and actor runtime.
Given this information, I hope it is clearer why the continuations don't have anything do do with "on what executor" since the resume is independent of that. We only signal "okey, resume please" and it is up to the callers' context to determine where the resuption has to happen -- i.e. the actor, or "any executor (e.g. some global one)" if called e.g. from some non actor context.
Sure. But this particular proposal covers functionality that allows converting completion handler APIs to async APIs without having to rewrite everything using actors. So I’m asking how this proposal will interact with APIs that take a queue in addition to a completion handler. Will the tasks jump back to the calling queue or will they continue on the queue they were called on? Or, in general, how do the proposed APIs interact with queues, as they aren’t mentioned at all.
They crash always, regardless if debug/release or any other configuration.
Yeah I was arguing for the same to be honest, so it's up to the core team to decide...
The motivation stated to only crash on the double-resume and not on the "forget-to-resume" was that the forget to resume would not provide a good diagnostic -- it is triggeded from deinit which can be any thread, and may be bard to locate what actually we forgot to resume. So double resume is definitely undefined behavior, but forgetting is "just" a memory leak... Somewhere I was arguing for at least offering some env variable or something to allow "please crash if not resumed"
In the pitch thread, I suggested the existing Result enum, so that we can use:
Result<T, Never> instead of separate non-throwing APIs.
resume(.success(value)) instead of resume(returning: value).
resume(.failure(error)) instead of resume(throwing: error).
Are the with*Continuation methods misnamed? The continuation is allowed to escape the given closure, unlike other with* methods in the standard library.
Could you elaborate on the non-actor context, or else comment on whether UI-based app development is likely to automatically funnel everything into a main-thread actor context?
This proposal looks great, but this point on resumption context outside actors seems pretty significant seeing as actors are currently still in the pitch phase.
Are we likely to start seeing a lot of boilerplate code along these lines in the near future?
let myResult = await getResult()
await DispatchQueue.asyncMain()
“UI stuff” means being on the “main actor” so it will always automatically resume on the main actor again.
No, there should not be any of such boilerplate as you suggest.
This proposal though is just the resuming — which is equal to calling an async function as to where the resumed code will run, so does not really play into this review.
We should be clear that this would be done by queue-hopping. The point of the recommendation that async APIs provide a queue argument is to avoid queue-hopping. That is, if you want the completion handler to resume on a particular queue then it's better if the caller of that handler enqueues it directly onto that queue rather than dispatching to another queue and requiring the completion handler to dispatch again to get to the queue it really wants to be on.
What async/await does is provide a mechanism where continuations can be called on their desired context (whether it be a queue or by some other means), but this API doesn't allow you to interact with that. So that means when you call resume you will probably be on the wrong queue to start with, and the executor will have to dispatch (or do something else) to get back to the right context. That's exactly what the APIs that take a queue are trying to avoid.
I don't think there's a general solution to this problem because executors for async/await aren't required to use GCD in their implementations. However, I would guess that most implementations (at least on Apple platforms) will use GCD, which means it might be nice if the continuation API provided a way to get a GCD queue that it would prefer to have resume called on. That would at least allow for more efficient implementations that avoid queue hopping when possible.
Callers of resume would not be required to call resume on that queue, but doing so would possibly be more efficient. Meanwhile, executors that don't use queues could just provide an arbitrary queue.
This would of course also require that the API for executors themselves to have a method to get an optional queue to dispatch continuations on so that when the continuation is created it could ask the executor for the right queue.
Is all of that worth it for an occasionally performance boost that only some lower-level APIs would benefit from? Probably not, unfortunately...
@ktoso Even without actual API to control our queueing, could you summarize the queue hopping that may occur when using the proposed APIs?
For example, Alamofire has many response* APIs that takes a closure as well a DispatchQueue parameters, using .main by default. Before fully adopting async throughout the library, offering a compatibility API would be a good idea. Take this simplest API:
let url = ...
let response = await AF.request(url).response()
response.map { ... }
If this code executes on a non-main queue, what happens? I would think that the response.map call happens on the same queue as the creating of the url, but it isn't clear. I think some mention of how DispatchQueues interact with this proposal is necessary.
)
.
.)
