Fix Observer.unschedule hang on linux when called with watch of an unmounted path

[IlayRosenberg]

Description

I experienced a hang on Observer.unschedule with the following flow:

1. Scheduling a watch for a folder in a container namespace (/proc/pid/root/...),
2. Removing the container (docker rm -f ..)
3. Unscheduling the watch

After some debugging I noticed that the InotifyBuffer thread is stuck on Inotify.read_events, specifically on os.read(self._inotify_fd, event_buffer_size).
As far as I can tell, usually when a watch is unscheduled, inotify_rm_watch is called after the thread stop event is set, which then generates an inotify event that causes os.read to return, and eventually the thread to finish its execution (after should_keep_running returns false). Otherwise, if the watch folder is deleted, an IN_DELETE_SELF event is generated which causes the thread to stop (by setting deleted_self).
However, if the entire filesystem containing the watched object was unmounted, an IN_UNMOUNT event is generated, which isn't handled the same way (or at all). Additionally, in all these cases, an IN_IGNORED event should follow, which is what I ended up handling instead of the specific IN_UNMOUNT.

All I did is set deleted_self if an IN_IGNORED event was received with the Inotify's path, to make sure the thread doesn't hang on the next os.read call, and it seem to have fixed the issue.

TL;DR

Observer.unschedule hangs if scheduled on a folder in a removed container. I added handling of IN_IGNORED events which should stop the thread on such cases.

[BoboTiG]

Thanks! Do you think writing a test case is possible for that?
Also do you mind adding a line + your GitHub nickname in the changelog? :)

[IlayRosenberg]

Thanks! Do you think writing a test case is possible for that? Also do you mind adding a line + your GitHub nickname in the changelog? :)

Done 🙂

[IlayRosenberg]

@BoboTiG Changed the logic a bit to keep event reporting the same as it was before this change (so not adding any IN_IGNORE events to the queue). Additionally I think the check for is_delete_self is redundant now, because it should be followed by an is_ignored event. Should I remove it?

[IlayRosenberg]

Sorry about the formatting issues, I fixed them and ran flake8 locally to make sure everything is formatted correctly

[IlayRosenberg]

Hey @BoboTiG, is there anything else you want me to do here? I don't think the failing tests are related to this PR - they are macos tests (which shouldn't be affected by my changes, since I only modified inotify), plus I saw the same tests failing on other (merged) PRs. Let me know if I'm missing something

[BoboTiG]

Sorry for the delay @IlayRosenberg, I'll review your PR shortly.

[BoboTiG]

Is it required to remove that line?

[IlayRosenberg]

Yes, because otherwise the IN_IGNORE event isn't added to the event_list and I can't handle it in InotifyBuffer.run

[BoboTiG]

Hm I wanted to edit your changes, especially the changelog ones, but it is not allowed. Would you mind rebasing the PR on master 🙏 ?

[IlayRosenberg]

Hm I wanted to edit your changes, especially the changelog ones, but it is not allowed. Would you mind rebasing the PR on master 🙏 ?

I rebased and updated the changelog

[BoboTiG]

Thanks a lot @IlayRosenberg :)