Fix race on timer in util.queue. #1285
Conversation
We're hitting an issue occasionally where Tabulator will stall out, which seems to be caused by the queue getting stuck during 'sleep()', likely because the timer channel has already been pulled from in the second select case, and the wait on receiving a value from the timer channel after timer.Stop() fails will block indefinitely. Instead, ensure that timer.Stop() is definitely called, but don't bother to drain or close the channel. (Note that timer.Stop() doesn't stop the channel, but it should be cleaned up due to being out of scope once sleep() completes).
| start := time.Now() | ||
| select { | ||
| case <-q.signal: | ||
| if !sleep.Stop() { | ||
| <-sleep.C |
There was a problem hiding this comment.
Good catch. Based on the documentation for Timer.Stop this will block forever if the timer already fired.
It returns ... false if the timer has already expired or been stopped.
This seems like a pretty rare race since the select statement also watches the timer, but if q.sleep() is called when there is already a signal pending this would happen reliably.
There was a problem hiding this comment.
Yeah, the logs are not logging every occurence of this since they stick to INFO level, but I suspect given the frequency of updates that sleep and rouse are both happening quite frequently in production. (I'm guessing steady increase in traffic has made this more likely over time, so we're seeing it more often recently).
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cjwagner, michelle192837 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
We're hitting an issue occasionally where Tabulator will stall out, which seems to be caused by the queue getting stuck during 'sleep()', likely because the timer channel has already been pulled from in the second select case, and the wait on receiving a value from the timer channel after timer.Stop() fails will block indefinitely.
Instead, ensure that timer.Stop() is definitely called, but don't bother to drain or close the channel. (Note that timer.Stop() doesn't stop the channel, but it should be cleaned up due to being out of scope once sleep() completes).