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

Prevent infinite loop when watching circular symlinks on Linux #47

Merged
merged 3 commits into from
Dec 13, 2017

Conversation

Tyriar
Copy link
Contributor

@Tyriar Tyriar commented Dec 12, 2017

A follow up from #41, this pulls in that change and then stops the watcher in the test and tidies it to match the code style in the rest of the file.

While verifying the test I notice that it doesn't actually test the problem since the files are watched in an async worker so the main thread doesn't get blocked by the infinite loop. I figure it's still useful to keep.

Note that I haven't reviewed the code thoroughly coming from the commits in #41, I'll leave that up to the expert 😄

Fixes #40

Nathan Sobo and others added 3 commits October 27, 2017 16:48
This prevents an endless loop in the face of recursive symbolic links.

Signed-off-by: Max Brunsfeld <maxbrunsfeld@gmail.com>
Signed-off-by: Max Brunsfeld <maxbrunsfeld@gmail.com>
@implausible
Copy link
Contributor

implausible commented Dec 13, 2017

@Tyriar thanks! I will check on this tonight :). As for the test, is stopping the watcher not sufficient for proving that we are able to exit the thread? I think stop is currently sync'd with the exit of the watching thread, so if it were looping endlessly, that may prevent a proper shutdown of that thread.

edit: Also, sorry that it can take some time to get to these. I try my best to checkin at least once a month, but I have a lot of duties at work these days.

@Tyriar
Copy link
Contributor Author

Tyriar commented Dec 13, 2017

I ran the test against master and it passed so I don't think the watch.stop() call at the end it's sufficient. https://github.com/Axosoft/nsfw/pull/47/files#diff-dd49db10a44aabd917dea709ba9a7353R438

@implausible implausible merged commit 277aeff into Axosoft:master Dec 13, 2017
@Tyriar Tyriar deleted the 40_infinite_linux_symlink branch December 13, 2017 15:28
@Tyriar
Copy link
Contributor Author

Tyriar commented Dec 28, 2017

@implausible any plans for a release soon?

@Tyriar
Copy link
Contributor Author

Tyriar commented Jan 22, 2018

Ping @implausible

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.

2 participants