Prevent race condition when JobBuffer#push is run in a tight loop and workers are waiting on jobs #318
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
oeoeaio
commented
Dec 8, 2021
lib/que/job_buffer.spec.rb
Outdated
| raise "Deadlock" | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
This test only take 0.1 seconds to run and fails on master about 50% of the time. We tried to develop a more targeted test for this issue but everything we tried ended up reaching into the internal implementation to such an extent that the test became very brittle.
I would be interested to know if anyone has a different hit rate for the Deadlock exception on master on their machine.
oeoeaio
force-pushed
the
fix-push-race-condition
branch
from
b149985
to
308784b
Compare
|
Given we've fixed the test subsequently, I'm going to remove it from this PR and merge the test in another |
… workers are waiting on jobs Co-authored-by: Maddy Markovitz <maddy.markovitz@greensync.com.au> Co-authored-by: Brendan Weibrecht <brendan@weibrecht.net.au>
ZimbiX
force-pushed
the
fix-push-race-condition
branch
from
308784b
to
89d4fa0
Compare
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.
This PR resolves an issue identified by @ebeigarts while crafting a test for #285 (see comment).
NOTE: this PR depends on #285, but we thought we would merge that PR separately rather than attempting to mash these changes in too.
Problem
The crux of the issue is that when
JobBuffer#pushis run on a tight loop (as it is in the test provided by @ebeigarts, which we have committed here, with credit) while there are workers waiting on jobs, there is a race condition between:PriorityQueue#popand reducing the@waitingcounterJobBuffer#pushre-entering that method and evaluatingpq.waiting_count.timesIf 2 happens before 1 (which seems unlikely but not impossible), then then too many jobs will be pushed into the priority queue, leading to the
Que.assert(waiting_count > 0)assertion inProrityQueue#pushfailing.It's unclear whether this is all that likely to happen in reality, but given there is proof that the race condition is possible we thought it better to try and resolve it.
Resolution
By shifting the push loop into the priority worker itself (
#populate), we are able to utilize the priority queue's own mutex to ensure the@waitingcount does not change while the push loop is running.We have made
PriorityQueue#_pusha private unsynchonized method since it is now only used via#populate.It's unclear whether there are performance impacts from making this change (workers are blocked from picking up jobs which we are pushing new jobs into each queue), but there is no measurable difference in the time taken for the test (about 2.3s with the original version which moves 40k jobs through the buffer) on my machine with or without this change.