Don't suspend/resume the thread while holding an exclusive inout reference #198
Conversation
|
This is a more targeted alternative to #194 |
jakepetroules
force-pushed
the
eng/PR-workqueue-fix
branch
6 times, most recently
from
9cf060a to
27e73ed
Compare
…rence We pass WorkQueue.queue 'by reference' through inout parameters and across thread suspension points. We noticed a stale value issue in release builds when passing Array with inout, likely due to the copy-on-write mechanism. To avoid stale values after a thread resumes from suspension, sleep the worker thread only after releasing the exclusive inout reference, and re-obtain it after resuming. Resolves swiftlang#182
jakepetroules
force-pushed
the
eng/PR-workqueue-fix
branch
from
27e73ed to
c535f36
Compare
| #if canImport(WinSDK) | ||
| SleepConditionVariableCS(self.waitCondition, mutex, INFINITE) | ||
| #else | ||
| pthread_cond_wait(self.waitCondition, mutex) |
There was a problem hiding this comment.
This doesn't look right. pthread_cont_wait and pretty much all other low-level condition variable implementations I have used can wake up spuriously. So usually they need to be in a loop. I.e. while condition(&queue) { instead of if condition(&queue)
There was a problem hiding this comment.
I don't think that alone would be correct in this particular case. If I change this to while, it'll hang during shutdown(), because shutdown() clears the queue (making it empty) and then wakes the condition variable. It must proceed in that case rather than go back to sleep.
That said, I think you may be right that something is generally amiss because where dequeue() is called it looks like it could end up terminating the work queue early if the condition variable was woken spuriously, since returning nil can't distinguish "no work items right now" versus "entered shutdown mode".
There was a problem hiding this comment.
Yeah, that doesn't seem good. Condition variables without a while are almost never right.
And yes, you'll need to add some sentinel to distinguish shutdown vs. empty
|
Thank you for fix! Could we expect patch release included this fix? |
4 participants
We pass WorkQueue.queue 'by reference' through inout parameters and across thread suspension points. We noticed a stale value issue in release builds when passing Array with inout, likely due to the copy-on-write mechanism. To avoid stale values after a thread resumes from suspension, sleep the worker thread only after releasing the exclusive inout reference, and re-obtain it after resuming.
Resolves #182