Switch from group.waitForAll() to group.next()
#254
Conversation
# 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
left a comment
phausler
left a comment
There was a problem hiding this comment.
If we are sure that the isEmpty does not introduce races (which from our side doesn't seem to have any) this looks fine to me.
|
@swift-ci test |
ktoso
left a comment
ktoso
left a comment
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 👍
4 participants
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.