Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don’t block on write if we’ve been killed #163

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

paulsc
Copy link

@paulsc paulsc commented Dec 7, 2019

Hi - I've been debugging this issue and fixed it with the changes in this pull request. I'm a bit new to go - so forgive me If I misunderstand, but here's what happened.

Quick context first: I am using hpcloud/tail in my component, called "CsvTail" which tails a file, parses the CSV records in that file, and only stops when it encounters the NUL byte at the beginning of a line. The CSV files are streamed and written by another service, which uses the NUL byte to indicate the end of the file.

The code would sometimes deadlock when calling tail.Stop(). Stop() internally calls tail.Wait(), which waits for the tomb to be declared as dead.

What happens is that another line comes in after my read of the NUL byte, but before my Stop() call, causing the tail library to block on this line.

Tail would never reach the end of tailFileSync() where it checks for tail.Dying(), returns, and sets the tomb as Done().

My fix is to add a select and "cancel" the write if the tomb enters dying state.

Not sure if this is right though let me know if I'm misunderstanding or misusing the library.

Best,

Paul

@nxadm
Copy link

nxadm commented Dec 7, 2019

Sadly, this repo seems dormant for more than 1,5 year. I forked it because I use it in my code. I integrated the PRs waiting merging and refreshed the code. A PR was recently merged that seems to address the same issue: nxadm/tail#7

There were some other fixes that prevent not showing the last line in the buffer while waiting on a newline. If you have the time, please do review the changes.

@paulsc
Copy link
Author

paulsc commented Dec 8, 2019

Ah ok. Thanks for update. I wish the owner of the hpcloud repo would add a note in the readme to point to yours!

@paulsc
Copy link
Author

paulsc commented Dec 8, 2019

FYI I tried switching to your fork @nxadm, but encountered an issue where I would get incomplete lines sometimes so had to switch back. I wish I could give more detail but don't have time to debug further right now.

@nxadm
Copy link

nxadm commented Dec 9, 2019

FYI I tried switching to your fork @nxadm, but encountered an issue where I would get incomplete lines sometimes so had to switch back. I wish I could give more detail but don't have time to debug further right now.

After the merging of the PRs all tests passed. Once you have the time I would love know which kind of logs trigger this error (e.g. what is a incomplete line). It may be related to the fix of the bug where the last line of a log wasn't sent until new messages arrived, e.g. because this last line didn't have a new line). Feel free to contact me once you have the time.

@nxadm
Copy link

nxadm commented Jan 25, 2021

Reviewing the code, I saw the change you proposed is already the nxadm/tail repo, probably by another PR. If you find the time, please test if the problem still occurs, and if it does please reopen an issue at https://github.com/nxadm/tail/issues (if possible with the data file and a small test).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants