Skip to content

Fix deadlock when stopping on non-empty file/buffer #7

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

Merged
merged 1 commit into from
Sep 23, 2019

Conversation

lukedirtwalker
Copy link
Contributor

At the same time also fix some lint issues (deadcode and comments).

Fixes hpcloud/tail#93

At the same time also fix some lint issues (deadcode and comments).

Fixes hpcloud/tail#93
@nxadm
Copy link
Owner

nxadm commented Sep 19, 2019

Thank you, I'll review the changes as soon as possible.

I had a quick look and the changes in the test file reminded me of my wish to replace the tomb v1 dependency by tomb v2 (or something else context based). v2 does not have the Done() method:

	- tail.Done()
	// tail.close()
	+ tail.Stop()

If you have any thoughts about that, your input is always welcome

@lukedirtwalker
Copy link
Contributor Author

Hi I don't have deep opinions about which lib to use. But I think the tomb thingy should not be embedded. Making the methods public is problematic. I.e. calling Done from the outside can lead to a panic because it is already called in the code itself.

Once it is removed from the public API you would also be more flexible to choose whatever lib you want.

I will wrap this lib in a tail library for my use case and at first I used a context approach in my wrapping library. But it didn't work out since it lead to racy tests, since I really want to wait until the Stop method returns. Not sure if using a context approach in the lib would work out. But if it does it would certainly be nice.

@nxadm nxadm merged commit b6839f5 into nxadm:master Sep 23, 2019
@nxadm
Copy link
Owner

nxadm commented Sep 23, 2019

Thanks for PR! I'll add a note to the issue tracking as well.

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.

tail.Stop() throws "all goroutines are asleep - deadlock!"
2 participants