Skip to content

[Concurrency] improve cancellation handler to not hop and use caller execution context #80753

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ktoso
Copy link
Contributor

@ktoso ktoso commented Apr 11, 2025

The problem is that even with the #isolation parameter the non-Sendable
async closure operation still would potentially hop off the caller
isolation. We introduced this change because the plan was the
non-Sendable closure would run on the isolated parameter's isolation,
but that's not actually the case:

Instead, we can use the @execution(caller) on the function and closure,
in order to guarantee there is no hop between those at all, and
developers can trust that adding this cancellation handler will not
cause any unexpected isolation changes and hops. The API was always
documented to not hop as we execute the operation, so this brings the
correct and expected behavior.

resolves rdar://140110775

@ktoso ktoso requested a review from a team as a code owner April 11, 2025 07:24
@ktoso ktoso force-pushed the wip-improve-withTaskCancellationHandler-with-caller-execution branch 2 times, most recently from de601db to 1303d99 Compare April 11, 2025 07:28
@ktoso
Copy link
Contributor Author

ktoso commented Apr 12, 2025

This is blocked by incorrect behavior of the caller execution on a closure rdar://149107104

@ktoso ktoso added the concurrency Feature: umbrella label for concurrency language features label Apr 17, 2025
@ktoso ktoso force-pushed the wip-improve-withTaskCancellationHandler-with-caller-execution branch 2 times, most recently from a583630 to 9ea3293 Compare April 30, 2025 02:13
@ktoso
Copy link
Contributor Author

ktoso commented Apr 30, 2025

Now blocked on rdar://150017382

@ktoso
Copy link
Contributor Author

ktoso commented May 15, 2025

@swift-ci please smoke test

@@ -63,6 +63,8 @@ endif()
list(APPEND SWIFT_RUNTIME_CONCURRENCY_SWIFT_FLAGS
"-enable-experimental-feature"
"IsolatedAny"
"-enable-experimental-feature"
"ExecutionAttribute"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can remove this now

@gottesmm
Copy link
Contributor

@swift-ci smoke test

1 similar comment
@gottesmm
Copy link
Contributor

gottesmm commented Jul 7, 2025

@swift-ci smoke test

@ktoso
Copy link
Contributor Author

ktoso commented Jul 8, 2025

Good news, this was now unblocked by #82858

@ktoso ktoso force-pushed the wip-improve-withTaskCancellationHandler-with-caller-execution branch 2 times, most recently from f0ecf2d to 25a963e Compare July 8, 2025 09:28
The problem is that even with the #isolation parameter the non-Sendable
async closure operation _still_ would potentially hop off the caller
isolation. We introduced this change because the plan was the
non-Sendable closure would run on the isolated parameter's isolation,
but that's not actually the case:

Instead, we can use the @execution(caller) on the function and closure,
in order to guarantee there is no hop between those at all, and
developers can trust that adding this cancellation handler will not
cause any unexpected isolation changes and hops. The API was always
documented to not hop as we execute the operation, so this brings the
correct and expected behavior.

resolves rdar://140110775

move to nonisolated(nonsending)

[Concurrency] latest cancellation handler is emit into client; no ABI entry
@ktoso ktoso force-pushed the wip-improve-withTaskCancellationHandler-with-caller-execution branch from 25a963e to be2b2ad Compare July 8, 2025 09:28
#if !$Embedded
@backDeployed(before: SwiftStdlib 6.0)
#endif
@_alwaysEmitIntoClient
Copy link
Contributor Author

@ktoso ktoso Jul 8, 2025

Choose a reason for hiding this comment

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

going with just AEIC those; we keep changing concurrency annotations and those methods are so small that AEIC for them is going to be fine anyway (as most of concurrency API already are like this)

defer { unsafe _taskRemoveCancellationHandler(record: record) }

return try await operation()
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This way we stay on the caller for as long as possible, and the closure is also invoked on it. This improves the way we don't hop away from the caller unnecessarily anymore

@ktoso
Copy link
Contributor Author

ktoso commented Jul 8, 2025

@swift-ci please smoke test

@ktoso
Copy link
Contributor Author

ktoso commented Jul 8, 2025

This is almost unblocked, we need to fix throwing nonisolated nonsending (caller isolated) closures to not lose isolation -- rdar://155313349

And there seems to be a problem with noncopyable types and nonisolated nonsending (caller isolated) closures as well; It shows up in cancellation_handler_only_once.swift:20 as a error: copy of noncopyable typed value. -- rdar://155316655

Giving it a run to see if anything else

@ktoso
Copy link
Contributor Author

ktoso commented Jul 8, 2025

@swift-ci please smoke test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
concurrency Feature: umbrella label for concurrency language features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants