Skip to content

Commit 5e10d36

Browse files
committed
Make sure that cancelling a task group does not invoke cancellation
handler of parent task that created the group Change comment in TaskGroup.swift to enforce that only parent task can call cancelAll on the group Add tests to verify mutating of task group in child tasks will fail Radar-Id: rdar://problem/86346865
1 parent 7b10313 commit 5e10d36

File tree

7 files changed

+83
-98
lines changed

7 files changed

+83
-98
lines changed

include/swift/ABI/TaskGroup.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
#define SWIFT_ABI_TASK_GROUP_H
1919

2020
#include "swift/ABI/Task.h"
21+
#include "swift/ABI/TaskStatus.h"
2122
#include "swift/ABI/HeapObject.h"
2223
#include "swift/Runtime/Concurrency.h"
2324
#include "swift/Runtime/Config.h"
@@ -46,6 +47,9 @@ class alignas(Alignment_TaskGroup) TaskGroup {
4647
// Add a child task to the group. Always called with the status record lock of
4748
// the parent task held
4849
void addChildTask(AsyncTask *task);
50+
51+
// Provide accessor for task group's status record
52+
TaskGroupTaskStatusRecord *getTaskRecord();
4953
};
5054

5155
} // end namespace swift

stdlib/public/Concurrency/TaskGroup.cpp

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -449,6 +449,10 @@ static TaskGroup *asAbstract(TaskGroupImpl *group) {
449449
return reinterpret_cast<TaskGroup*>(group);
450450
}
451451

452+
TaskGroupTaskStatusRecord * TaskGroup::getTaskRecord() {
453+
return asImpl(this)->getTaskRecord();
454+
}
455+
452456
// =============================================================================
453457
// ==== initialize -------------------------------------------------------------
454458

stdlib/public/Concurrency/TaskGroup.swift

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -373,8 +373,8 @@ public struct TaskGroup<ChildTaskResult: Sendable> {
373373
/// If you add a task to a group after canceling the group,
374374
/// that task is canceled immediately after being added to the group.
375375
///
376-
/// There are no restrictions on where you can call this method.
377-
/// Code inside a child task or even another task can cancel a group.
376+
/// This method can only be called by the parent task that created the task
377+
/// group.
378378
///
379379
/// - SeeAlso: `Task.isCancelled`
380380
/// - SeeAlso: `TaskGroup.isCancelled`

stdlib/public/Concurrency/TaskStatus.cpp

Lines changed: 6 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -485,41 +485,6 @@ static void performCancellationAction(TaskStatusRecord *record) {
485485
// FIXME: allow dynamic extension/correction?
486486
}
487487

488-
/// Perform any cancellation actions required by the given record.
489-
static void performGroupCancellationAction(TaskStatusRecord *record) {
490-
switch (record->getKind()) {
491-
// We only need to cancel specific GroupChildTasks, not arbitrary child tasks.
492-
// A task may be parent to many tasks which are not part of a group after all.
493-
case TaskStatusRecordKind::ChildTask:
494-
return;
495-
496-
case TaskStatusRecordKind::TaskGroup: {
497-
auto groupChildRecord = cast<TaskGroupTaskStatusRecord>(record);
498-
// Since a task can only be running a single task group at the same time,
499-
// we can always assume that the group record which we found is the one
500-
// we're intended to cancel child tasks for.
501-
//
502-
// A group enforces that tasks can not "escape" it, and as such once the group
503-
// returns, all its task have been completed.
504-
for (AsyncTask *child: groupChildRecord->children()) {
505-
swift_task_cancel(child);
506-
}
507-
return;
508-
}
509-
510-
// All other kinds of records we handle the same way as in a normal cancellation
511-
case TaskStatusRecordKind::Deadline:
512-
case TaskStatusRecordKind::CancellationNotification:
513-
case TaskStatusRecordKind::EscalationNotification:
514-
case TaskStatusRecordKind::Private_RecordLock:
515-
performCancellationAction(record);
516-
return;
517-
}
518-
519-
// Other cases can fall through here and be ignored.
520-
// FIXME: allow dynamic extension/correction?
521-
}
522-
523488
SWIFT_CC(swift)
524489
static void swift_task_cancelImpl(AsyncTask *task) {
525490
SWIFT_TASK_DEBUG_LOG("cancel task = %p", task);
@@ -543,17 +508,15 @@ SWIFT_CC(swift)
543508
static void swift_task_cancel_group_child_tasksImpl(TaskGroup *group) {
544509
// Acquire the status record lock.
545510
//
546-
// We purposefully DO NOT make this a cancellation by itself.
547-
// We are cancelling the task group, and all tasks it contains.
548-
// We are NOT cancelling the entire parent task though.
511+
// Guaranteed to be called from the context of the parent task that created
512+
// the task group once we have #40616
549513
auto task = swift_task_getCurrent();
550514
withStatusRecordLock(task, LockContext::OnTask,
551515
[&](ActiveTaskStatus &status) {
552-
// Carry out the cancellation operations associated with all
553-
// the active records.
554-
for (auto cur: status.records()) {
555-
performGroupCancellationAction(cur);
556-
}
516+
// We purposefully DO NOT make this a cancellation by itself.
517+
// We are cancelling the task group, and all tasks it contains.
518+
// We are NOT cancelling the entire parent task though.
519+
performCancellationAction(group->getTaskRecord());
557520
});
558521
}
559522

test/Concurrency/Runtime/async_taskgroup_cancel_from_inside_child.swift

Lines changed: 0 additions & 53 deletions
This file was deleted.
Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
// RUN: %target-run-simple-swift( -Xfrontend -disable-availability-checking %import-libdispatch -parse-as-library) | %FileCheck %s
2+
3+
// REQUIRES: executable_test
4+
// REQUIRES: concurrency
5+
// REQUIRES: libdispatch
6+
7+
// rdar://76038845
8+
// REQUIRES: concurrency_runtime
9+
// UNSUPPORTED: back_deployment_runtime
10+
11+
func test_taskGroup_cancelAll() async {
12+
13+
await withTaskCancellationHandler {
14+
await withTaskGroup(of: Int.self, returning: Void.self) { group in
15+
group.spawn {
16+
await Task.sleep(3_000_000_000)
17+
let c = Task.isCancelled
18+
print("group task isCancelled: \(c)")
19+
return 0
20+
}
21+
22+
group.cancelAll() // Cancels the group but not the task
23+
_ = await group.next()
24+
}
25+
} onCancel : {
26+
print("parent task cancel handler called")
27+
}
28+
29+
// CHECK-NOT: parent task cancel handler called
30+
// CHECK: group task isCancelled: true
31+
// CHECK: done
32+
print("done")
33+
}
34+
35+
@available(SwiftStdlib 5.1, *)
36+
@main struct Main {
37+
static func main() async {
38+
await test_taskGroup_cancelAll()
39+
}
40+
}
Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
// RUN: %target-typecheck-verify-swift -disable-availability-checking
2+
// REQUIRES: concurrency
3+
4+
@available(SwiftStdlib 5.1, *)
5+
func test_taskGroup_cancelAll() async {
6+
await withTaskGroup(of: Int.self, returning: Void.self) { group in
7+
group.spawn {
8+
await Task.sleep(3_000_000_000)
9+
let c = Task.isCancelled
10+
print("group task isCancelled: \(c)")
11+
return 0
12+
}
13+
14+
group.spawn {
15+
group.cancelAll() //expected-warning{{capture of 'group' with non-sendable type 'TaskGroup<Int>' in a `@Sendable` closure}}
16+
//expected-error@-1{{reference to captured parameter 'group' in concurrently-executing code}}
17+
return 0
18+
}
19+
group.spawn { [group] in
20+
group.cancelAll() //expected-warning{{capture of 'group' with non-sendable type 'TaskGroup<Int>' in a `@Sendable` closure}}
21+
return 0
22+
}
23+
_ = await group.next()
24+
}
25+
26+
print("done")
27+
}

0 commit comments

Comments
 (0)