🍒[5.9][TaskGroup] Fix unlock order, add missing detaches and add more assertions #67819
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
reenable async_taskgroup_discarding_dontLeak.swift [DiscardingTaskGroup] Properly detach when LAST task is failed, and prior failure was already stored [TaskGroup] Must detach discarded task, THEN unlock before resume waiting revamping locking scheme, test this a bunch stabilize println based test a bit more against timing re-enable tsan test: async_taskgroup_next reenable async_taskgroup_next_on_pending disable debugging tricks unlock test: async_taskgroup_asynciterator_semantics make use of unreachable cleanups cleanup for freestanding mode
ktoso
force-pushed
the
pick-taks-group-unlock-before-run
branch
from
1ab7b64 to
2e6381f
Compare
|
@swift-ci please test |
ktoso
added
🍒 release cherry pick
ktoso
changed the title
al45tair
approved these changes
Aug 9, 2023
| auto waitingContext = | ||
| static_cast<TaskFutureWaitAsyncContext *>( | ||
| waitingTask->ResumeContext); | ||
| // Run the task. |
There was a problem hiding this comment.
Nit: this comment isn't entirely accurate :-) (same as on the review of the original PR).
mikeash
approved these changes
Aug 9, 2023
|
Added the docs-only code review change 1195955 from the original PR review. Might as well give this another full test run as well 👍 |
|
@swift-ci please test |
airspeedswift
approved these changes
Aug 11, 2023
ktoso
added a commit
to ktoso/swift
that referenced
this pull request
Aug 11, 2023
…before-run 🍒[5.9][TaskGroup] Fix unlock order, add missing detaches and add more assertions
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: A task group resumes the "waiting task" in numerous situations. Currently tasks were scheduled and then the group was unlocked -- this can lead to races between the scheduled task and the group unlock and unpredictable behavior. Instead, we must unlock the group and THEN schedule the waiting task on order to avoid potential use-after free of the lock (as the unlock() happens).
Risk: Medium, the change reorganizes code in order to allow us to unlock and THEN schedule the task. This forced some general refactoring in order to be able to get this pattern.
Reward Medium, resolves very rare crashes which could occur when just the right scheduling timing would happen. These issues are very rare, and have remained undetected until recently.
Review by: @mikeash @DougGregor
Testing: CI testing, enabled all task group tests for the first time in a long time and all passing consistently on all platforms.
Original PR: #67590
Radar: rdar://113331923 (test reenable rdar://113016918)
Related Radar: The following was the same issue however in a more crucial code path: rdar://113032582