[Concurrency] Adopt typed throws and nonisolated(nonsending) in withTaskCancellationHandler #85518
Conversation
…askCancellationHandler Replace the use of rethrows and #isolation in withTaskCancellationHandler with typed throws and nonisolated(nonsending), respectively. Fixes rdar://146901428.
|
@swift-ci please smoke test |
|
@swift-ci please smoke test |
| public func withTaskCancellationHandler<T, E>( | ||
| operation: () async throws(E) -> T, | ||
| onCancel handler: @Sendable () -> Void | ||
| ) async throws(E) -> T { |
There was a problem hiding this comment.
do we have any tests that exercise the case described here?
func next(isolation: isolated (any Actor)? = #isolation) async {
MainActor.assertIsolated() // passes
await withTaskCancellationHandler {
MainActor.assertIsolated() // fails!
} onCancel: { }
}
@MainActor
func callOnMain() async {
await next()
}is the outcome in that scenario expected to be different now? or will the same issue occur depending on how the closure isolation is inferred?
There was a problem hiding this comment.
This has been a long standing bug. We always "promised" that these with methods would never lose isolation however this didn't actually work correctly with the isolated parameter...
This is finally bringing the correct behavior to these APIs. All the with methods have the same issue and we'll change all of them to now give the behavior we've always promised to begin with
There was a problem hiding this comment.
maybe i'm confused as to what issue is being targeted here. my understanding is that the existing signature (prior to a conversion to nonisolated(nonsending)) already inherits the caller's actor isolation (via the defaulted isolated param). so there already should never be an isolation crossing when calling the function. but IIUC closures can be inferred as non-isolated if they are declared in an instance-isolated context but do not capture the isolated parameter.
it seems like this change alone (ignoring typed throws) will not alter the behavior of this API – if the operation closure is inferred to be non-isolated, then it will be seen as an isolation crossing, and introduce a possibly-unexpected executor hop. is the 'full fix' dependent on the NonisolatedNonsendingByDefault feature also being enabled?
There was a problem hiding this comment.
Well, that is the (very long standing) bug, the async closure still would hop:
await withTaskCancellationHandler {
MainActor.assertIsolated() // fails!
this is very much incorrect and should have never been happening to hop off, but it does. The way closure isolation works just isn't correct here and the "real" solution is to move all these with... APIs to nonisolated nonsending.
it seems like this change alone (ignoring typed throws) will not alter the behavior of this API
It does change behavior; it corrects the unexpected hopping off in certain scenarios -- one of which you've just pasted, that is incorrect and we must fix it; it breaks the safety contract of the withTaskCancellationHandler function
There was a problem hiding this comment.
It does change behavior; it corrects the unexpected hopping off in certain scenarios -- one of which you've just pasted, that is incorrect and we must fix it; it breaks the safety contract of the withTaskCancellationHandler function
focusing specifically on the change of the function signature to nonisolated(nonsending) – that does not affect how the operation closure is inferred does it?
admittedly i have not applied these changes locally to experiment, but if you make a function with a similar signature, it does not appear that without the NonisolatedNonsendingByDefault feature enabled the 'unexpected hop' goes away, as the operation closures appear to continue to be inferred to have no isolation. here are some examples demonstrating this: https://swift.godbolt.org/z/chK56d97q.
is that class of examples, the only way in which isolation can be 'lost' or are there others?
anyway, i apologize for the intrusion as this is probably not the appropriate venue to have this sort of discussion. to briefly return to my original motivation – if this change is anticipated to fix a known bug, it seems prudent to have tests that demonstrate the fix works as intended.
|
@swift-ci please smoke test |
|
@swift-ci please smoke test Windows |
|
swiftlang/swift-foundation#1602 @swift-ci please smoke test Linux |
|
@swift-ci please smoke test Windows |
3 similar comments
|
@swift-ci please smoke test Windows |
|
@swift-ci please smoke test Windows |
|
@swift-ci please smoke test Windows |
| @export(implementation) | ||
| nonisolated(nonsending) | ||
| public func withTaskCancellationHandler<T, E>( | ||
| operation: () async throws(E) -> T, |
There was a problem hiding this comment.
Shouldn't this closure also be nonisolated(nonsending) otherwise when calling it we might need to insert defensive hops?
| operation: () async throws(E) -> T, | |
| operation: nonisolated(nonsending) () async throws(E) -> T, |
| nonisolated(nonsending) | ||
| public func withTaskCancellationHandler<T, E>( | ||
| operation: () async throws(E) -> T, | ||
| onCancel handler: @Sendable () -> Void |
There was a problem hiding this comment.
Can't this closure be sending since it is only send to the task/thread that cancels this task so it is never executed concurrently itself just concurrent to the caller of the withTaskCancellationHandler.
| @available(SwiftStdlib 5.1, *) | ||
| @export(implementation) | ||
| nonisolated(nonsending) | ||
| public func withTaskCancellationHandler<T, E>( |
There was a problem hiding this comment.
NIT: Could we give these generic types a better name so that they show up nicely in docs or would that be API breaking?
| public func withTaskCancellationHandler<T, E>( | |
| public func withTaskCancellationHandler<Return, Failure: Error>( |
5 participants
Replace the use of rethrows and #isolation in
withTaskCancellationHandler with typed throws and
nonisolated(nonsending), respectively.
Fixes rdar://146901428.