From 059e5dd15eeb82ce4afe4160556b4fd8c130e55b Mon Sep 17 00:00:00 2001 From: Konrad 'ktoso' Malawski Date: Mon, 14 Apr 2025 17:01:30 +0900 Subject: [PATCH 1/3] [Concurrency] Parent task cancellation must cancel task group itself. Seems that during refactorings of child cancellations we somehow missed also cancelling the group itself. It seems we did not have good test coverage of the addTaskUnlessCancelled somehow and thus this slipped through. This adds a regression test for addTaskUnlessCancelled and fixes how we handle the cancellation effect in TaskStatus. resolves #80789 resolves rdar://149177600 --- include/swift/ABI/TaskGroup.h | 6 ++ stdlib/public/Concurrency/TaskGroup.cpp | 4 ++ stdlib/public/Concurrency/TaskStatus.cpp | 8 ++- .../async_taskgroup_addUnlessCancelled.swift | 64 +++++++++++++++++++ 4 files changed, 81 insertions(+), 1 deletion(-) create mode 100644 test/Concurrency/Runtime/async_taskgroup_addUnlessCancelled.swift diff --git a/include/swift/ABI/TaskGroup.h b/include/swift/ABI/TaskGroup.h index b1c821d8e1cba..03f7e101cce06 100644 --- a/include/swift/ABI/TaskGroup.h +++ b/include/swift/ABI/TaskGroup.h @@ -44,6 +44,12 @@ class alignas(Alignment_TaskGroup) TaskGroup { /// Checks the cancellation status of the group. bool isCancelled(); + /// Only mark the task group as cancelled, without performing the follow-up + /// work of cancelling all the child tasks. + /// + /// Returns true if the group was already cancelled before this call. + bool statusCancel(); + // Add a child task to the task group. Always called while holding the // status record lock of the task group's owning task. void addChildTask(AsyncTask *task); diff --git a/stdlib/public/Concurrency/TaskGroup.cpp b/stdlib/public/Concurrency/TaskGroup.cpp index 6de5d06c8e347..054ae3f38b562 100644 --- a/stdlib/public/Concurrency/TaskGroup.cpp +++ b/stdlib/public/Concurrency/TaskGroup.cpp @@ -1166,6 +1166,10 @@ bool TaskGroup::isCancelled() { return asBaseImpl(this)->isCancelled(); } +bool TaskGroup::statusCancel() { + return asBaseImpl(this)->statusCancel(); +} + // ============================================================================= // ==== offer ------------------------------------------------------------------ diff --git a/stdlib/public/Concurrency/TaskStatus.cpp b/stdlib/public/Concurrency/TaskStatus.cpp index 38cc28b45535e..f9c5dfc184dee 100644 --- a/stdlib/public/Concurrency/TaskStatus.cpp +++ b/stdlib/public/Concurrency/TaskStatus.cpp @@ -781,6 +781,7 @@ void swift::_swift_taskGroup_detachChild(TaskGroup *group, /// The caller must guarantee that this is called while holding the owning /// task's status record lock. void swift::_swift_taskGroup_cancelAllChildren(TaskGroup *group) { + assert(group->isCancelled() && "Expected task group to be cancelled when cancelling all child tasks."); // Because only the owning task of the task group can modify the // child list of a task group status record, and it can only do so // while holding the owning task's status record lock, we do not need @@ -825,7 +826,12 @@ static void performCancellationAction(TaskStatusRecord *record) { // under the synchronous control of the task that owns the group. case TaskStatusRecordKind::TaskGroup: { auto groupRecord = cast(record); - _swift_taskGroup_cancelAllChildren(groupRecord->getGroup()); + auto group = groupRecord->getGroup(); + auto wasAlreadyCancelled = group->statusCancel(); + if (wasAlreadyCancelled) { + return; + } + _swift_taskGroup_cancelAllChildren(group); return; } diff --git a/test/Concurrency/Runtime/async_taskgroup_addUnlessCancelled.swift b/test/Concurrency/Runtime/async_taskgroup_addUnlessCancelled.swift new file mode 100644 index 0000000000000..16ee240405658 --- /dev/null +++ b/test/Concurrency/Runtime/async_taskgroup_addUnlessCancelled.swift @@ -0,0 +1,64 @@ +// RUN: %target-run-simple-swift( -target %target-swift-5.1-abi-triple -parse-as-library) | %FileCheck %s + +// REQUIRES: executable_test +// REQUIRES: concurrency + +// rdar://76038845 +// REQUIRES: concurrency_runtime +// UNSUPPORTED: back_deployment_runtime + +import Dispatch + +@available(SwiftStdlib 6.0, *) +func test_withTaskGroup_addUnlessCancelled() async throws { + let task = Task { + await withTaskGroup(of: Void.self) { group in + print("Inner: Sleep...") + try? await Task.sleep(nanoseconds: 2_000_000_000) + print("Inner: Task.isCancelled: \(Task.isCancelled)") + + let added = group.addTaskUnlessCancelled { + print("Added Task! Child Task.isCancelled: \(Task.isCancelled)") + } + print("Inner: Task added = \(added)") // CHECK: Task added = false + } + } + + try? await Task.sleep(nanoseconds: 1_000_000) + print("Outer: Cancel!") + task.cancel() + print("Outer: Cancelled") + + await task.value +} + +@available(SwiftStdlib 6.0, *) +func test_withDiscardingTaskGroup_addUnlessCancelled() async throws { + let task = Task { + await withDiscardingTaskGroup { group in + print("Inner: Sleep...") + try? await Task.sleep(nanoseconds: 2_000_000_000) + print("Inner: Task.isCancelled: \(Task.isCancelled)") + + let added = group.addTaskUnlessCancelled { + print("Added Task! Child Task.isCancelled: \(Task.isCancelled)") + } + print("Inner: Task added = \(added)") // CHECK: Task added = false + } + } + + try? await Task.sleep(nanoseconds: 1_000_000) + print("Outer: Cancel!") + task.cancel() + print("Outer: Cancelled") + + await task.value +} + +@available(SwiftStdlib 6.0, *) +@main struct Main { + static func main() async { + try! await test_withTaskGroup_addUnlessCancelled() + try! await test_withDiscardingTaskGroup_addUnlessCancelled() + } +} From 29b8e271e1331f59a5b394afc1c7816ef82fc08b Mon Sep 17 00:00:00 2001 From: Konrad 'ktoso' Malawski Date: Mon, 14 Apr 2025 17:24:08 +0900 Subject: [PATCH 2/3] [Concurrency] Refactor _swift_taskGroup_cancelAllChildren -> _cancel This way we do the right thing always when cancelling the group; and we MAY visit the child tasks if we have to. --- stdlib/public/Concurrency/TaskGroup.cpp | 8 ++++---- stdlib/public/Concurrency/TaskPrivate.h | 8 ++++---- stdlib/public/Concurrency/TaskStatus.cpp | 20 ++++++++------------ 3 files changed, 16 insertions(+), 20 deletions(-) diff --git a/stdlib/public/Concurrency/TaskGroup.cpp b/stdlib/public/Concurrency/TaskGroup.cpp index 054ae3f38b562..f4dd349551f4f 100644 --- a/stdlib/public/Concurrency/TaskGroup.cpp +++ b/stdlib/public/Concurrency/TaskGroup.cpp @@ -2120,8 +2120,8 @@ bool TaskGroupBase::cancelAll(AsyncTask *owningTask) { // Cancel all the child tasks. TaskGroup is not a Sendable type, // so cancelAll() can only be called from the owning task. This - // satisfies the precondition on cancelAllChildren_unlocked(). - _swift_taskGroup_cancelAllChildren_unlocked(asAbstract(this), owningTask); + // satisfies the precondition on cancel_unlocked(). + _swift_taskGroup_cancel_unlocked(asAbstract(this), owningTask); return true; } @@ -2130,8 +2130,8 @@ SWIFT_CC(swift) static void swift_task_cancel_group_child_tasksImpl(TaskGroup *group) { // TaskGroup is not a Sendable type, and so this operation (which is not // currently exposed in the API) can only be called from the owning - // task. This satisfies the precondition on cancelAllChildren_unlocked(). - _swift_taskGroup_cancelAllChildren_unlocked(group, swift_task_getCurrent()); + // task. This satisfies the precondition on cancel_unlocked(). + _swift_taskGroup_cancel_unlocked(group, swift_task_getCurrent()); } // ============================================================================= diff --git a/stdlib/public/Concurrency/TaskPrivate.h b/stdlib/public/Concurrency/TaskPrivate.h index 2235e7a687b23..c6151c27217cd 100644 --- a/stdlib/public/Concurrency/TaskPrivate.h +++ b/stdlib/public/Concurrency/TaskPrivate.h @@ -89,16 +89,16 @@ AsyncTask *_swift_task_clearCurrent(); /// Set the active task reference for the current thread. AsyncTask *_swift_task_setCurrent(AsyncTask *newTask); -/// Cancel all the child tasks that belong to `group`. +/// Cancel the task group and all the child tasks that belong to `group`. /// /// The caller must guarantee that this is called while holding the owning /// task's status record lock. -void _swift_taskGroup_cancelAllChildren(TaskGroup *group); +void _swift_taskGroup_cancel(TaskGroup *group); -/// Cancel all the child tasks that belong to `group`. +/// Cancel the task group and all the child tasks that belong to `group`. /// /// The caller must guarantee that this is called from the owning task. -void _swift_taskGroup_cancelAllChildren_unlocked(TaskGroup *group, +void _swift_taskGroup_cancel_unlocked(TaskGroup *group, AsyncTask *owningTask); /// Remove the given task from the given task group. diff --git a/stdlib/public/Concurrency/TaskStatus.cpp b/stdlib/public/Concurrency/TaskStatus.cpp index f9c5dfc184dee..12e5b9187058d 100644 --- a/stdlib/public/Concurrency/TaskStatus.cpp +++ b/stdlib/public/Concurrency/TaskStatus.cpp @@ -776,12 +776,13 @@ void swift::_swift_taskGroup_detachChild(TaskGroup *group, }); } -/// Cancel all the child tasks that belong to `group`. +/// Cancel the task group and all the child tasks that belong to `group`. /// /// The caller must guarantee that this is called while holding the owning /// task's status record lock. -void swift::_swift_taskGroup_cancelAllChildren(TaskGroup *group) { - assert(group->isCancelled() && "Expected task group to be cancelled when cancelling all child tasks."); +void swift::_swift_taskGroup_cancel(TaskGroup *group) { + (void) group->statusCancel(); + // Because only the owning task of the task group can modify the // child list of a task group status record, and it can only do so // while holding the owning task's status record lock, we do not need @@ -790,10 +791,10 @@ void swift::_swift_taskGroup_cancelAllChildren(TaskGroup *group) { swift_task_cancel(childTask); } -/// Cancel all the child tasks that belong to `group`. +/// Cancel the task group and all the child tasks that belong to `group`. /// /// The caller must guarantee that this is called from the owning task. -void swift::_swift_taskGroup_cancelAllChildren_unlocked(TaskGroup *group, +void swift::_swift_taskGroup_cancel_unlocked(TaskGroup *group, AsyncTask *owningTask) { // Early out. If there are no children, there's nothing to do. We can safely // check this without locking, since this can only be concurrently mutated @@ -802,7 +803,7 @@ void swift::_swift_taskGroup_cancelAllChildren_unlocked(TaskGroup *group, return; withStatusRecordLock(owningTask, [&group](ActiveTaskStatus status) { - _swift_taskGroup_cancelAllChildren(group); + _swift_taskGroup_cancel(group); }); } @@ -826,12 +827,7 @@ static void performCancellationAction(TaskStatusRecord *record) { // under the synchronous control of the task that owns the group. case TaskStatusRecordKind::TaskGroup: { auto groupRecord = cast(record); - auto group = groupRecord->getGroup(); - auto wasAlreadyCancelled = group->statusCancel(); - if (wasAlreadyCancelled) { - return; - } - _swift_taskGroup_cancelAllChildren(group); + _swift_taskGroup_cancel(groupRecord->getGroup()); return; } From 60a197856fea5a8aafa9cade9b2a0bfe41189a23 Mon Sep 17 00:00:00 2001 From: Konrad 'ktoso' Malawski Date: Mon, 14 Apr 2025 19:25:02 +0900 Subject: [PATCH 3/3] [Distributed] avoid sleep in addUnlessCancelled test --- .../async_taskgroup_addUnlessCancelled.swift | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/test/Concurrency/Runtime/async_taskgroup_addUnlessCancelled.swift b/test/Concurrency/Runtime/async_taskgroup_addUnlessCancelled.swift index 16ee240405658..12bfa0abc2641 100644 --- a/test/Concurrency/Runtime/async_taskgroup_addUnlessCancelled.swift +++ b/test/Concurrency/Runtime/async_taskgroup_addUnlessCancelled.swift @@ -3,28 +3,24 @@ // REQUIRES: executable_test // REQUIRES: concurrency -// rdar://76038845 // REQUIRES: concurrency_runtime // UNSUPPORTED: back_deployment_runtime -import Dispatch - @available(SwiftStdlib 6.0, *) func test_withTaskGroup_addUnlessCancelled() async throws { let task = Task { await withTaskGroup(of: Void.self) { group in print("Inner: Sleep...") - try? await Task.sleep(nanoseconds: 2_000_000_000) + try? await Task.sleep(for: .seconds(60)) // we'll never actually wait 10 seconds, as this will be woken up by cancel print("Inner: Task.isCancelled: \(Task.isCancelled)") let added = group.addTaskUnlessCancelled { print("Added Task! Child Task.isCancelled: \(Task.isCancelled)") } - print("Inner: Task added = \(added)") // CHECK: Task added = false + print("Inner: Task added = \(added)") // CHECK: Inner: Task added = false } } - try? await Task.sleep(nanoseconds: 1_000_000) print("Outer: Cancel!") task.cancel() print("Outer: Cancelled") @@ -37,17 +33,16 @@ func test_withDiscardingTaskGroup_addUnlessCancelled() async throws { let task = Task { await withDiscardingTaskGroup { group in print("Inner: Sleep...") - try? await Task.sleep(nanoseconds: 2_000_000_000) + try? await Task.sleep(for: .seconds(60)) // we'll never actually wait 10 seconds, as this will be woken up by cancel print("Inner: Task.isCancelled: \(Task.isCancelled)") let added = group.addTaskUnlessCancelled { print("Added Task! Child Task.isCancelled: \(Task.isCancelled)") } - print("Inner: Task added = \(added)") // CHECK: Task added = false + print("Inner: Task added = \(added)") // CHECK: Inner: Task added = false } } - try? await Task.sleep(nanoseconds: 1_000_000) print("Outer: Cancel!") task.cancel() print("Outer: Cancelled")