fix thread safety issue in package collections #3136
Conversation
motivation: a failing test hinted to a thread safety issue, which was then diagnosed to an incorrect lock on an array. this prompted creating a thread safe utility for array to avoid such cases changes: * create a thread-safe utlitiy for array * replace array + lock with the new utility * add test
tomerd
requested review from
abertelrud,
aciidgh,
friedbunny and
neonichu
as code owners
| sources.forEach { source in | ||
| self.refreshCollectionFromSource(source: source) { refreshResult in | ||
| lock.withLock { refreshResults.append(refreshResult) } | ||
| if refreshResults.count == (lock.withLock { sources.count }) { |
There was a problem hiding this comment.
this was the issue that prompted the PR. I saw a few PRs failing on PackageCollectionsTests.testHappyRefresh inconsistently so I was hunting for a race condition (interesting TSAN did not report this). the issue here is that the lock is on sources instead of refreshResults. so in addition to fixing this, I decided to create this new utility for arrays so we dont have to mess with locks
There was a problem hiding this comment.
I fixed this coding mistake in #3108 though didn't anticipate it causing race condition. Anyway, 👍
|
@swift-ci please test |
|
@swift-ci please smoke test |
tomerd
changed the title
|
@swift-ci please smoke test |
| } | ||
| } | ||
| } | ||
|
|
||
| /// Thread-safe value boxing structure |
There was a problem hiding this comment.
| /// Thread-safe value boxing structure | |
| /// Thread-safe value boxing structure |
| sources.forEach { source in | ||
| self.refreshCollectionFromSource(source: source) { refreshResult in | ||
| lock.withLock { refreshResults.append(refreshResult) } | ||
| if refreshResults.count == (lock.withLock { sources.count }) { |
There was a problem hiding this comment.
I fixed this coding mistake in #3108 though didn't anticipate it causing race condition. Anyway, 👍
motivation: a failing test hinted at a thread safety issue, which was then diagnosed to be an incorrect lock on an array. this prompted creating a thread safe utility for array to avoid such cases changes: * create a thread-safe utlitiy for array * replace array + lock with the new utility * add test
motivation: a failing test hinted to a thread safety issue, which was then diagnosed to an incorrect lock on an array. this prompted creating a thread safe utility for array to avoid such cases
changes: