Skip to content

[Concurrency] Adopt typed throws and nonisolated(nonsending) in withTaskCancellationHandler #85518

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+38 −10
Open

[Concurrency] Adopt typed throws and nonisolated(nonsending) in withTaskCancellationHandler #85518

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
babe6f2
[Concurrency] Adopt typed throws and nonisolated(nonsending) in withT…
DougGregor Nov 14, 2025
b7ec3a0
Drop extraneous `isolation` argument
DougGregor Nov 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 23 additions & 3 deletions stdlib/public/Concurrency/TaskCancellation.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,10 +74,29 @@ import Swift
/// Therefore, if a cancellation handler must acquire a lock, other code should
/// not cancel tasks or resume continuations while holding that lock.
@available(SwiftStdlib 5.1, *)
@export(implementation)
Copy link
Collaborator

@AnthonyLatsis AnthonyLatsis Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add a TODO/FIXME with a reminder to replace this with @backDeployed(before: Swift 6.4) at some point?

nonisolated(nonsending)
public func withTaskCancellationHandler<T, E>(
Copy link
Member

@FranzBusch FranzBusch Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: Could we give these generic types a better name so that they show up nicely in docs or would that be API breaking?

Suggested change
public func withTaskCancellationHandler<T, E>(
public func withTaskCancellationHandler<Return, Failure: Error>(

operation: () async throws(E) -> T,
Copy link
Member

@FranzBusch FranzBusch Nov 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this closure also be nonisolated(nonsending) otherwise when calling it we might need to insert defensive hops?

Suggested change
operation: () async throws(E) -> T,
operation: nonisolated(nonsending) () async throws(E) -> T,

onCancel handler: @Sendable () -> Void
Copy link
Member

@FranzBusch FranzBusch Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

) async throws(E) -> T {
Copy link
Contributor

@jamieQ jamieQ Nov 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@ktoso ktoso Nov 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@jamieQ jamieQ Nov 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@ktoso ktoso Nov 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@jamieQ jamieQ Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// unconditionally add the cancellation record to the task.
// if the task was already cancelled, it will be executed right away.
let record = unsafe Builtin.taskAddCancellationHandler(handler: handler)
defer { unsafe Builtin.taskRemoveCancellationHandler(record: record) }

return try await operation()
}

#if !$Embedded
@backDeployed(before: SwiftStdlib 6.0)
#endif
public func withTaskCancellationHandler<T>(
@available(SwiftStdlib 6.0, *)
@usableFromInline
@abi(func withTaskCancellationHandler<T>(
operation: () async throws -> T,
onCancel handler: @Sendable () -> Void,
isolation: isolated (any Actor)?,
) async rethrows -> T)
func __abi_withTaskCancellationHandler<T>(
operation: () async throws -> T,
onCancel handler: @Sendable () -> Void,
isolation: isolated (any Actor)? = #isolation
Expand All @@ -89,6 +108,7 @@ public func withTaskCancellationHandler<T>(

return try await operation()
}
#endif

// Note: hack to stage out @_unsafeInheritExecutor forms of various functions
// in favor of #isolation. The _unsafeInheritExecutor_ prefix is meaningful
Expand Down
2 changes: 1 addition & 1 deletion stdlib/public/Observation/Sources/Observation/Observations.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,7 @@ public struct Observations<Element: Sendable, Failure: Error>: AsyncSequence, Se
}, onCancel: {
// ensure to clean out our continuation uon cancellation
State.cancel(state, id: id)
}, isolation: iterationIsolation)
})
return try await trackEmission(isolation: iterationIsolation, state: state, id: id)
}
} catch {
Expand Down
16 changes: 12 additions & 4 deletions test/Concurrency/async_cancellation.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,23 @@ struct SomeFile: Sendable {
func close() {}
}

enum HomeworkError: Error {
case dogAteIt
}

@available(SwiftStdlib 5.1, *)
func test_cancellation_withTaskCancellationHandler(_ anything: Any) async -> PictureData? {
let handle: Task<PictureData, Error> = .init {
let file = SomeFile()

return await withTaskCancellationHandler {
await test_cancellation_guard_isCancelled(file)
} onCancel: {
file.close()
do throws(HomeworkError) {
return try await withTaskCancellationHandler { () throws(HomeworkError) in
await test_cancellation_guard_isCancelled(file)
} onCancel: {
file.close()
}
} catch .dogAteIt {
return PictureData.value("...")
}
}

Expand Down
4 changes: 2 additions & 2 deletions test/api-digester/stability-concurrency-abi.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,8 @@ Func withCheckedThrowingContinuation(function:_:) has parameter 0 type change fr
Func withCheckedThrowingContinuation(function:_:) has parameter 1 type change from (_Concurrency.CheckedContinuation<τ_0_0, any Swift.Error>) -> () to Swift.String

// #isolation adoption for cancellation handlers; old APIs are kept ABI compatible
Func withTaskCancellationHandler(operation:onCancel:) has been renamed to Func withTaskCancellationHandler(operation:onCancel:isolation:)
Func withTaskCancellationHandler(operation:onCancel:) has mangled name changing from '_Concurrency.withTaskCancellationHandler<A>(operation: () async throws -> A, onCancel: @Sendable () -> ()) async throws -> A' to '_Concurrency.withTaskCancellationHandler<A>(operation: () async throws -> A, onCancel: @Sendable () -> (), isolation: isolated Swift.Optional<Swift.Actor>) async throws -> A'
Func withTaskCancellationHandler(operation:onCancel:) has been renamed to Func __abi_withTaskCancellationHandler(operation:onCancel:isolation:)
Func withTaskCancellationHandler(operation:onCancel:) has mangled name changing from '_Concurrency.withTaskCancellationHandler<A>(operation: () async throws -> A, onCancel: @Sendable () -> ()) async throws -> A' to '_Concurrency.__abi_withTaskCancellationHandler<A>(operation: () async throws -> A, onCancel: @Sendable () -> (), isolation: isolated Swift.Optional<Swift.Actor>) async throws -> A'

// #isolated was adopted and the old methods kept: $ss31withCheckedThrowingContinuation8function_xSS_yScCyxs5Error_pGXEtYaKlF
Func withCheckedContinuation(function:_:) has been renamed to Func withCheckedContinuation(isolation:function:_:)
Expand Down