Avoid deadlocks when closing follower - #15
Open
seanlinsley wants to merge 6 commits into
Open
Conversation
Reading from a closed channel always returns immediately, and when close gets invoked waiting Go routines are instantly unblocked. This is a more reliable mechanism than using a buffered channel for closeCh, which could theoretically block on a third call to the Follower Close() method. Use sync.Once to ensure the channel is only closed once, since subsequent close calls would otherwise panic.
The file is assumed to be owned and only modified by the internally running tail Go routine, and closing the file directly causes a data race, as reported by the Go runtime. Whilst we could wrap the file in a mutex to allow eager closure, we can rely on the tail routine itself to perform the close, especially after switching to using channel closure instead of a buffered channel.
This ensures that we react correctly to the follower being closed, instead of potentially blocking on writing to the lines channel. The logic here matches what we do when we notice the close channel being closed later in the same routine.
Previously we re-opened the file exactly once after a rename or remove event, assuming an atomic log rotation operation. However, as is likely in practice, if the rotation program had not yet created the new file, the open failed and the follower exited, permanently stopping the tail. To fix, retry every 50ms for up to 1 second, but only when we actually get a failure due to the file not existing, as was already intended to be implemented and documented in a code comment.
This test method was previously only checking errors that occur immediately when the log tail is set up, and was doing so whilst also actively reading from Lines in a separate Go routine. That is not safe to do due to the lack of a supporting synchronization primitive. Instead, first read all lines, and then check for errors. This depends on the recent TestRenameCreate fix which ensures we don't error out mid-test in some cases (which was previously silently ignored). As reported when running the tests with the "-race" argument.
Author
|
Note: pganalyze#1 extended this PR with additional changes |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Closedeadlocks whenfollowis blocked in the inner read loop (ReadByteswaiting for data). ThecloseChselect is only in the outer loop, soCloseblocks forever on the unbufferedcloseChsend.This is addressed by making
closeCha buffered channel to prevent a deadlock, and avoiding potential dangling file descriptors by immediately closing the file instead of deferring tocloseCh.