Switch from group.waitForAll() to group.next()
#254
Merged
Conversation
This file contains 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
# Motivation Swift 5.8 is including a change to how `group.waitForAll()` is working. It now properly waits for all tasks to finish even if one of the tasks throws. We have used `group.waitForAll()` in multiple places and need to change this code accordingly. # Modification Switch code from `group.waitForAll()` to `group.next()`. # Result This fixes a few stuck tests that we have seen when running against development snapshots.
|
I can confirm this fixes all the test issues alluded to in #253 on Android AArch64. On linux x86_64, maybe it's because I'm running on a single-core VPS, but I still see issues. If you don't see any problems on a beefier linux box, probably unrelated and you can go ahead with this. |
phausler
approved these changes
Feb 27, 2023
|
@swift-ci test |
ktoso
approved these changes
Feb 28, 2023
There was a problem hiding this comment.
This is LGTM and I clarified semantics of the waitForAll now in Swift itself: swiftlang/swift#63956
The implementation actually is pretty much the same in Swift, a loop over isEmpty so this will be ok -- it is not racy since only the parent task can be adding tasks 👍
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.
Motivation
Swift 5.8 is including a change to how
group.waitForAll()is working. It now properly waits for all tasks to finish even if one of the tasks throws. We have usedgroup.waitForAll()in multiple places and need to change this code accordingly.Modification
Switch code from
group.waitForAll()togroup.next().Result
This fixes a few stuck tests that we have seen when running against development snapshots.