Resolve race between task creation and concurrent escalation and cancellation. #40606

rokhinip · 2021-12-17T05:16:04Z

This change has two parts to it:

Add in a new interface (addStatusRecordWithChecks) for adding task
status records that also takes in a function ref. This function ref will
be used to evaluate if current state of the parent task has any changes
that need to be propagated to the child task that has been created.

This is necessary to prevent the following race between task creation
and concurrent cancellation and escalation:

(a) Parent task create child task. It does lazy relaxed loads on its own
state while doing so and propagates this state to the child.
(b) Child task is created but has not been attached to the parent
task/task group.
(c) Parent task gets cancelled by another thread.
(d) Child task gets linked into the parent’s task status records but no
reevaluation has happened to account for changes that might have happened to
the parent after (a).

Move status record management functions from the
Runtime/Concurrency.h to TaskPrivate.h. Remove any corresponding
overrides that are no longer needed. Remove unused tryAddStatusRecord
method whose functionality is provided by addStatusRecordWithChecks.

Rdar: rdar://86347801

rokhinip · 2021-12-17T05:24:32Z

Doh, just hit a conflict with @rjmccall's changes. I'm going to have to take some time to rework this.

rokhinip · 2021-12-17T07:44:05Z

@swift-ci please test

ktoso

LGTM, just confirming my understanding in a few places and minor nitpicks.

include/swift/Runtime/Concurrency.h

stdlib/public/Concurrency/Task.cpp

ktoso · 2021-12-18T00:23:22Z

stdlib/public/Concurrency/TaskStatus.cpp

+              /*failure*/ std::memory_order_relaxed)) {
+        return true;
+      } else {
+        /* Retry */


thx for explicit comments like that; big fan of such style 👍

include/swift/Runtime/Concurrency.h

stdlib/public/Concurrency/AsyncLet.cpp

stdlib/public/Concurrency/TaskStatus.cpp

rokhinip · 2021-12-18T02:58:35Z

@swift-ci please smoke test and merge it

stdlib/public/Concurrency/Task.cpp

stdlib/public/Concurrency/TaskGroup.cpp

stdlib/public/CompatibilityOverride/CompatibilityOverrideConcurrency.def

rjmccall · 2021-12-18T04:03:40Z

stdlib/public/Concurrency/TaskPrivate.h

+///
+/// Returns false if the task has been cancelled.
+SWIFT_EXPORT_FROM(swift_Concurrency)
+SWIFT_CC(swift) bool swift_task_removeStatusRecord(TaskStatusRecord *record);


If these are no longer public ABI, (1) they shouldn't be exported and (2) please give them names that don't sound like they're in the ABI. For example, this one could just be removeTaskStatusRecord without any prefixes.

Ah. Okay I didn't realize the swift_ prefix was for exported functions only. I'll adjust them.

So while I was fixing this, I realized that we do want to be able to export them from the concurrency library to be able to write unit tests against them - see unittests/runtime/TaskStatus.cpp.

So shall I keep to the regular swift_ prefixed names and export the functions or should we do something else since we're talking about exporting for the sake of testing?

rjmccall · 2021-12-18T04:05:48Z

stdlib/public/Concurrency/TaskPrivate.h

+SWIFT_CC(swift)
+bool swift_task_addStatusRecordWithChecks(
+    TaskStatusRecord *record,
+    llvm::function_ref<bool(ActiveTaskStatus)> testAddRecord);


If this is the only function for adding a status record, it can just be addTaskStatusRecord, and it just takes an update function because that's what it does.

stdlib/public/Concurrency/TaskPrivate.h

stdlib/public/Concurrency/TaskStatus.cpp

rjmccall · 2021-12-18T04:13:29Z

stdlib/public/Concurrency/TaskStatus.cpp

+   * child task since we know that it won't have any task status records and
+   * cannot be accessed by anyone else since it hasn't been linked in yet.
+   * Avoids the extra logic in `swift_task_cancel` and `swift_task_escalate`
+   */


We generally use // comments unless we're specifically doing aCallWith(/*an inline comment before an*/ argument).

Fix these comments, please. Doc comments before functions use ///, inline comments use //.

rjmccall · 2021-12-18T04:16:08Z

stdlib/public/Concurrency/TaskStatus.cpp

+  if (parentStatus.isStoredPriorityEscalated()) {
+    newChildTaskStatus = newChildTaskStatus.withEscalatedPriority(
+        parentStatus.getStoredPriority());
+  }


I'm not sure it's safe to rely on isStoredPriorityEscalated here, since it can be cleared asynchronously. You should just set the priority of the child task to the priority of the parent. You should also update the priority in the Job header.

How can it be cleared? Isn't escalation permanent on a task?

I'd expect a task to be created at say, UT priority, and then if it gets escalated to IN priority, to have the escalated flag be permanently set on the task status.

Oh okay reading further, I think I misunderstood the semantics of the isEscalated flag. Okay you're right, I need to change this.

rjmccall · 2021-12-18T04:18:44Z

stdlib/public/Concurrency/Task.cpp

+                                            * needs to be idempotent */
+                                         }
+                                         return true;
+                                       });


As a general rule, we format around lambdas as if they were a sub-statement of the call, so this would be more like:

bool fireHandlerNow = false; swift_task_addStatusRecordWithChecks(record, [&](ActiveTaskStatus parentStatus) { fireHandlerNow = parentStatus.isCancelled(); return true; // Add the status record regardless });

rokhinip · 2021-12-21T03:52:25Z

@swift-ci please smoke test

stdlib/public/Concurrency/TaskGroup.cpp

stdlib/public/Concurrency/TaskPrivate.h

rjmccall · 2021-12-22T04:45:15Z

stdlib/public/Concurrency/TaskStatus.cpp

+   * child task since we know that it won't have any task status records and
+   * cannot be accessed by anyone else since it hasn't been linked in yet.
+   * Avoids the extra logic in `swift_task_cancel` and `swift_task_escalate`
+   */


Fix these comments, please. Doc comments before functions use ///, inline comments use //.

stdlib/public/Concurrency/TaskStatus.cpp

unittests/runtime/TaskStatus.cpp

This change has two parts to it: 1. Add in a new interface (addStatusRecordWithChecks) for adding task status records that also takes in a function ref. This function ref will be used to evaluate if current state of the parent task has any changes that need to be propagated to the child task that has been created. This is necessary to prevent the following race between task creation and concurrent cancellation and escalation: a. Parent task create child task. It does lazy relaxed loads on its own state while doing so and propagates this state to the child. b. Child task is created but has not been attached to the parent task/task group. c. Parent task gets cancelled by another thread. d. Child task gets linked into the parent’s task status records but no reevaluation has happened to account for changes that might have happened to the parent after (a). 2. Move status record management functions from the Runtime/Concurrency.h to TaskPrivate.h. Remove any corresponding overrides that are no longer needed. Remove unused tryAddStatusRecord method whose functionality is provided by addStatusRecordWithChecks. Radar-Id: rdar://problem/86347801

Radar-Id: rdar://problem/86347801

rokhinip · 2021-12-31T11:49:29Z

@swift-ci please test

swift-ci · 2021-12-31T15:05:04Z

Build failed
Swift Test Linux Platform
Git Sha - e35eba0

rokhinip · 2022-01-10T02:49:29Z

@swift-ci Please test Windows platform

rokhinip · 2022-01-10T02:50:41Z

@swift-ci Please smoke test Linux platform

rokhinip · 2022-01-10T02:52:51Z

@swift-ci Please test Linux platform

ktoso

Gave it another skim, looks good afaics.

rokhinip requested review from ktoso and rjmccall December 17, 2021 05:16

rokhinip force-pushed the rokhinip/86347801-task-creation-escalation-race branch 2 times, most recently from 12a4af7 to 32d7a7f Compare December 17, 2021 07:36

ktoso approved these changes Dec 18, 2021

View reviewed changes

rokhinip force-pushed the rokhinip/86347801-task-creation-escalation-race branch from 32d7a7f to 4410226 Compare December 18, 2021 02:58

rokhinip mentioned this pull request Dec 18, 2021

5.6 [Concurrency] Resolve race between task creation and concurrent escalation and cancellation #40622

Closed

rjmccall reviewed Dec 18, 2021

View reviewed changes

rokhinip requested a review from rjmccall December 21, 2021 03:51

rjmccall requested changes Dec 22, 2021

View reviewed changes

rokhinip added 2 commits December 31, 2021 03:23

Remove old addChild and detachChild stuff from files

e35eba0

Radar-Id: rdar://problem/86347801

rokhinip force-pushed the rokhinip/86347801-task-creation-escalation-race branch from 8e5b4e5 to e35eba0 Compare December 31, 2021 11:24

rokhinip requested review from rjmccall and ktoso January 10, 2022 09:07

ktoso approved these changes Jan 12, 2022

View reviewed changes

rokhinip merged commit a4fe57f into main Jan 12, 2022

rokhinip deleted the rokhinip/86347801-task-creation-escalation-race branch January 12, 2022 04:14

rokhinip mentioned this pull request Jan 13, 2022

5.6 [Concurrency]: Resolve race between task creation and concurrent escalation and cancellation #40831

Closed

Resolve race between task creation and concurrent escalation and cancellation. #40606

Resolve race between task creation and concurrent escalation and cancellation. #40606

Uh oh!

Conversation

rokhinip commented Dec 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rokhinip commented Dec 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rokhinip commented Dec 17, 2021

Uh oh!

ktoso left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rokhinip commented Dec 18, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rokhinip Dec 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rokhinip Dec 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rokhinip commented Dec 21, 2021

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rokhinip commented Dec 31, 2021

Uh oh!

swift-ci commented Dec 31, 2021

Uh oh!

rokhinip commented Jan 10, 2022

Uh oh!

rokhinip commented Jan 10, 2022

Uh oh!

rokhinip commented Jan 10, 2022

Uh oh!

ktoso left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rokhinip commented Dec 17, 2021 •

edited

Loading

rokhinip commented Dec 17, 2021 •

edited

Loading

rokhinip Dec 22, 2021 •

edited

Loading

rokhinip Dec 21, 2021 •

edited

Loading