5.6 [Concurrency] Resolve race between task creation and concurrent escalation and cancellation #40622
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
|
@swift-ci please test |
ktoso
approved these changes
Dec 18, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description:
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.
Risk: Low. This adds extra synchronization to the path of adding status records
Reviewed by: @ktoso
Testing: Unit tests. Manually verified using hacked up unit test. This is difficult to test automatically since it involves a fairly intricate race condition.
Original PR: #40606
Rdar: rdar://86347801