-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce a dedicated proposal for with*Continuation
#1244
Conversation
dd99837
to
ff506d4
Compare
- Trap on multiple resumes, rather than merely logging - Add discussion of alternatives considered, such as trapping vs logging tradeoffs, and lack of additional handle API
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @jckarter !
One minor addition at the end if you'd like to provide another example, it's something that had people confused on the forums that might be useful to clarify.
proposals/ABCD-continuation.md
Outdated
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
proposals/ABCD-continuation.md
Outdated
a deinit would execute is not entirely predictable because of refcounting | ||
variability from ARC optimization. If `deinit` were made to trap, whether that | ||
trap is executed and when could vary with optimization level, which we | ||
don't think would lead to a good experience. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This convienced me that that'a fine default I guess -- I initially wanted to trap here too, but then again -- if I'm debugging such leak then I can write my own wrapper that does the ugly/unreliable deinit anyway.
proposals/ABCD-continuation.md
Outdated
cancel() | ||
} | ||
``` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may want to add this example too, it showcases that we can respect outside/parent task cancellation within an API implemented using withUnsafeContinuation
which some people on the forums were not sure if this is supported or not:
It is also possible for wrappers around callback based APIs to respect their parent/current tasks's cancellation, as follows:
func fetch(items: Int) async throws -> [Items] {
let worker = ...
return try Task.withCancellationHandler(
handler: { worker?.cancel() }
) {
return try await withUnsafeThrowingContinuation { c in
worker.work(
onNext: { value in c.resume(returning: value) },
onCancelled: { value in c.resume(throwing: CancellationError()) },
)
}
}
}
If tasks were allowed to have instances, which is under discussion in the structured concurrency proposal, it would also be possible to obtain the task in which the fetch(items:)
function was invoked and call isCanceled
on it whenever the insides of the withUnsafeThrowingContinuation would deem it worthwhile to do so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I'll add this example to the proposal text.
No description provided.