-
Notifications
You must be signed in to change notification settings - Fork 10.6k
Adopt new language features for withTaskCancellationHandler
#85096
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
base: main
Are you sure you want to change the base?
Adopt new language features for withTaskCancellationHandler
#85096
Conversation
|
@swift-ci please smoke test |
|
@swift-ci please test source compatibility |
|
Technically source breaking with the isolated param removal; i attempted to get away with it in the continuation APIs where anything else than caller would be incorrect; let's see what source compat suite has to say about these though... |
| // ) | ||
| @_silgen_name("$s4test28withTaskCancellationHandlerN9operation8onCancelxxyYaq_YKYTXE_yyYbXEtYaq_YKs5ErrorR_r0_lF") | ||
| public nonisolated(nonsending) func withTaskCancellationHandler<T, Failure: Error>( | ||
| operation: () async throws(Failure) -> sending T, |
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.
I don't think returning a sending value here is the right thing. There are scenarios where that is not possible and then you can't setup a cancellation handler. However, can we allow T to be ~Copyable here? We can express a type in the type system that allows us to move a disconnected value into and take it sending out of it again.
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.
Yeah I was getting very hung up trying to decide if/how to make withTaskCancellationHandler return sending. You are right, this isn't a reasonable change.
However, now I'm wondering it the operation argument even needs this to achieve the same goal?
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.
oh and also, the idea of using ~Copyable to disconnect something is an idea that's come up a few times and I think is really awesome.
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.
However, now I'm wondering it the operation argument even needs this to achieve the same goal?
What do you mean with same goal exactly?
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.
My hope is to enable something like this (just illustrative, not literally trying to make wrapper function). However, I would settle for the other two changes if we cannot make this happen easily.
nonisolated(nonsending) func withCancellableContinuation<T>(
_ body: (CheckedContinuation<T, Never>) -> Void,
onCancel handler: @Sendable () -> Void
) async -> sending T {
// The result cannot be passed out of this function because withTaskCancellationHandler does not have a sending return.
// However, don't we know that there cannot be an isolation boundary at this point? Or, perhaps sending is the wrong way to express that?
// ERROR: Returning a nonisolated(nonsending) task-isolated '@async @callee_guaranteed () -> (@out T, @error any Error)' value as a 'sending' result risks causing data races
await withTaskCancellationHandler(
operation: {
// this value is a sending return
await withCheckedContinuation { continuation in
body(continuation)
}
},
onCancel: handler
)
}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.
However, don't we know that there cannot be an isolation boundary at this point? Or, perhaps sending is the wrong way to express that?
That's correct but what we don't know at this point is if the return value is actually disconnected from any state isolated to the current isolation (either task or actor). It might be that the return value is somehow tied to an isolated state. If the return value would be sending then the value would be guaranteed to be disconnected but it would prohibit other valid patterns.
My current thinking is that we can not reasonable make all APIs return their value as sending and we also cannot reasonably add overloads on sending. This specifically doesn't work with generic code such as generic containers. That's why I think a type like this that allows developers to send a value into a disconnected region and take it sending out again will allow such patterns safely.
struct Disconnected<Value>: ~Copyable {
private var value: Value?
init(value: consuming sending Value) {
self.value = .some(value)
}
consuming func take() -> sending Value {
self.value.takeSending()!
}
}a167e6f to
d6a5754
Compare
This makes three changes to the
withTaskCancellationHandlerAPI.sendingresult from the operation bodynonisolated(nonsending)This also includes an attempt at doing this in both an ABI and source-compatible way. However, I have concerns about both of these areas, so right now this is just a draft.