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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this is functionally equivalent. The old code would restart the timer on every iteration. But it looks like this new pattern introduces a small race where the timer can fire just as a request is received. The timer internally sends to
raftIdleTimer.C
and on the next iteration we callReset
, but that doesn't clear the channel. I'm not familiar enough with this code to know if that race is innocuous.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch; that does seem like an issue. Now that I look closely, there are several other problems with the shutdown-on-idle behavior. A new request could be written to the channel after the decision has been made to shut down the goroutine, and even without any races the goroutine could stop while there are still outstanding requests that will try to write to the
done
channel.We have several instances of this pattern where we want a goroutine that goes away when it is idle (here,
gossip.infoStore.runCallbacks
, and my upcoming work on parallelizing raft execution). I'm going to try to refactor theinfoStore
pattern into something reusable.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reset
returnsfalse
if the timer had expired (in other words, it returnsfalse
if there's a value parked on the channel). Seems easy enough to mitigate: