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

inotify: Ignore any error sending the RenameTimeout event. #373

Merged

Conversation

jorendorff
Copy link
Contributor

@jorendorff jorendorff commented Dec 7, 2021

The unwrap() call here causes thread panics in our application. We notice them because we use set_panic_hook. I think ordinarily a panic here would be printed to stderr and otherwise ignored.

For us, I think the usual pattern is that several files are renamed at once, and as soon as we notice the first rename, we drop the watcher. Therefore, when this thread wakes up, it's trying to send an event to a queue that has been closed.

The `unwrap()` call here causes thread panics in our application. We notice
them because we use `set_panic_hook`. I think ordinarily a panic here would be
printed to stderr and otherwise ignored.

For us, I think the usual pattern is that several files are renamed at once,
and as soon as we notice the first rename, we drop the watcher. Therefore, when
this thread wakes up, it's trying to send an event to a queue that has been
closed.
@0xpr03
Copy link
Member

0xpr03 commented Dec 7, 2021

Hm, I think the proper fix is to shutdown the whole loop in that case.

@0xpr03
Copy link
Member

0xpr03 commented Dec 7, 2021

Though I wonder why you're the first to report this ? Maybe other people start it once and then only destroy it during shutdown ?

@jorendorff
Copy link
Contributor Author

Hm, I think the proper fix is to shutdown the whole loop in that case.

Maybe I'm confused here. I was thinking this error can only happen if the event loop has shut down.

This is a special, awkward bit of code that sleeps for 10 msec and then tries to send a message. All the other places where code sends a message to this queue, there's a comment that says

        // we expect the event loop to live and reply => unwraps must not panic

Here, we can't make that assumption; the way the code's written, there's an unavoidable race condition.

Though I wonder why you're the first to report this ? Maybe other people start it once and then only destroy it during shutdown ?

Quite possible. But also, the panic always happens in a thread created by inotify, and the thread has no further responsibilities, so (unless you have panic=abort set) it doesn't hurt anything for that thread to panic. Maybe folks are just ignoring it.

@0xpr03
Copy link
Member

0xpr03 commented Dec 7, 2021

Yeah right I totally missed that thread creation and thought I'm inside the event loop.

@0xpr03
Copy link
Member

0xpr03 commented Dec 7, 2021

Though in that case you might want to also prevent a crash when the waker doesn't react any more.

@jorendorff
Copy link
Contributor Author

Ah, right, that makes sense. Added that.

Copy link
Member

@0xpr03 0xpr03 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@0xpr03 0xpr03 merged commit 442316e into notify-rs:main Dec 7, 2021
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