[inotify] Add support for the IN_CLOSE_WRITE event #747

ysard · 2021-01-13T01:22:27Z

Hi, after testing #690 I found that the event was still not captured by the "inotify.InotifyObserver" observer. Here are my additions to this PR that got me on track. This should also close #93.

"Full" implementation of the capture of IN_CLOSE_NOWRITE & IN_CLOSE_WRITE for the detection of the closing files (only).
Unit tests

Please note that IN_CLOSE_NOWRITE is triggered very often with the IN_ISDIR mask; those events are filtered, see:

https://github.com/ysard/watchdog/blob/0de211c9864bce310a8fd9e5634e6a3e1ea3f9bc/src/watchdog/observers/inotify.py#L178-L182

Some additional events (IN_ACCESS, IN_OPEN) could easily be added in the future.
I don't know how far you want to go in the inotify support because it implies to make the lib less portable so I don't add anything more.

Waiting for your review :)

This reverts commit 69d61d0.

…nts gorakhargosh#690

BoboTiG · 2021-01-13T07:21:17Z

Great!

Could you rebase your branch to fix conflicts?

I did not look, but is it necessary for IN_CLOSE_NOWRITE to generate an event? If the file was not modified, then the event should not be sent. WDYT?

Conflicts: changelog.rst src/watchdog/observers/inotify.py

ysard · 2021-01-14T03:49:14Z

It's now synced with master. I also fixed a test that failed in Travis.

About the other point, it's a good question...

For the sake of completeness and if we stick to the strict definition of the event: these are the 2 masks that must be considered.
Moreover this is already provided here (in helper user-space events):

watchdog/src/watchdog/observers/inotify_c.py

Line 93 in 847a059

IN_CLOSE = IN_CLOSE_WRITE | IN_CLOSE_NOWRITE # Close.
It's hard to know if people also expect notification when a file closes after read-only; from what I read in the issues and on stackoverflow it's IN_CLOSE_WRITE that's currently the most lacking.

In practice, the use of IN_CLOSE_NOWRITE is probably uncommon and "can" generate a higher processing cost because the slightest file traversal by a program emits IN_CLOSE_NOWRITE | IN_ISDIR.

You will always find someone who needs an unimplemented event for a project...

The safest thing to do would be to let the user choose the mask he is interested in; however the Inotify class (and the keyword argument event_mask of its constructor) is not accessible from the API.

Proof of the overhead with IN_CLOSE_WRITE | IN_CLOSE_NOWRITE:

fd = open("./dir/hello", "w")
<InotifyEvent: src_path=b'./dir/hello', wd=8, mask=IN_CREATE, cookie=0, name=hello>
<InotifyEvent: src_path=b'./dir', wd=1, mask=IN_CLOSE_NOWRITE|IN_ISDIR, cookie=0, name=dir>
<InotifyEvent: src_path=b'./dir', wd=8, mask=IN_CLOSE_NOWRITE|IN_ISDIR, cookie=0, name=>
<InotifyEvent: src_path=b'./dir/hello', wd=8, mask=IN_CLOSE_NOWRITE, cookie=0, name=hello>

fd.write("a")
fd.flush()
<InotifyEvent: src_path=b'./dir/hello', wd=8, mask=IN_MODIFY, cookie=0, name=hello>
<InotifyEvent: src_path=b'./dir/hello', wd=8, mask=IN_CLOSE_NOWRITE, cookie=0, name=hello>

fd.close()
<InotifyEvent: src_path=b'./dir/hello', wd=8, mask=IN_CLOSE_WRITE, cookie=0, name=hello>


os.remove("./dir/hello")

<InotifyEvent: src_path=b'./dir/hello', wd=8, mask=IN_DELETE, cookie=0, name=hello>
<InotifyEvent: src_path=b'./dir', wd=1, mask=IN_CLOSE_NOWRITE|IN_ISDIR, cookie=0, name=dir>
<InotifyEvent: src_path=b'./dir', wd=8, mask=IN_CLOSE_NOWRITE|IN_ISDIR, cookie=0, name=>

With only IN_CLOSE_WRITE:

<InotifyEvent: src_path=b'./dir/hello', wd=8, mask=IN_CREATE, cookie=0, name=hello>
<InotifyEvent: src_path=b'./dir/hello', wd=8, mask=IN_MODIFY, cookie=0, name=hello>
<InotifyEvent: src_path=b'./dir/hello', wd=8, mask=IN_CLOSE_WRITE, cookie=0, name=hello>

Other note: As indicated in the doc, the observers are specific to each OS (https://pythonhosted.org/watchdog/api.html#watchdog.observers.Observer).

However, to give access to the event we are interested in here, it was necessary to modify the class watchdog.events.FileSystemEventHandler by adding the on_closed method.

This class is meant to be universal and this modification is not portable. As the issue #217 reminds us, we must warn the user that this method is only available under GNU/Linux.

=> Should I add a note in the docstring?

The current issue is a long-standing problem (#245, #184, #313, #280, even #119 for the IN_ACCESS event).
It is still possible for the user to achieve this by adding the desired methods if the lib does not offer them via monkey patching, but this is not ideal.

BoboTiG · 2021-01-14T10:16:44Z

IMO the IN_CLOSE_NOWRITE event will add too much noise. And the benefits of having the IN_CLOSE event is to be able to know when a file modification has been done for real.

Could you do that?
You could comment the code you added to support IN_CLOSE_NOWRITE to keep the track for later. And duplicate and adapt

watchdog/src/watchdog/observers/inotify_c.py

Line 93 in 847a059

IN_CLOSE = IN_CLOSE_WRITE | IN_CLOSE_NOWRITE # Close.

to remove IN_CLOSE_NOWRITE.

src/watchdog/events.py

BoboTiG · 2021-01-14T10:26:42Z

Could you add a line in the changelog (+ your GitHub nickename and @lukassup to our beloved contributors):

- [inotify] Add support for ``IN_CLOSE_WRITE`` events. A ``FileCloseEvent`` event will be fired. Note that ``IN_CLOSE_NOWRITE`` events are not handled to prevent much noise. (`#184 <https://github.com/gorakhargosh/watchdog/pull/184>`_, `#245 <https://github.com/gorakhargosh/watchdog/pull/245>`_, `#280 <https://github.com/gorakhargosh/watchdog/pull/280>`_, `#313 <https://github.com/gorakhargosh/watchdog/pull/313>`_, `#690 <https://github.com/gorakhargosh/watchdog/pull/690>`_)

src/watchdog/observers/inotify.py

ysard · 2021-01-15T00:14:53Z

It's done now ;)

BoboTiG · 2021-01-15T09:23:05Z

Thanks a lot @ysard 💪

* Fix installation of Python 3.7.9 on Windows (#705) * [windows] winapi.BUFFER_SIZE now defaults to 64000 (instead of 2048) To handle cases with lot of changes, this seems the highest safest value we can use. Note: it will fail with `ERROR_INVALID_PARAMETER` when it is greater than 64 KB and the application is monitoring a directory over the network. This is due to a packet size limitation with the underlying file sharing protocols. https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-readdirectorychangesw#remarks Also introduced `winapi.PATH_BUFFER_SIZE` (defaults to `2048`) to keep the old behavior with path-realted functions. * [mac] Regression fixes for native fsevents (#717) * [mac] Fix missing event_id attribute in fsevents (#722) * [mac] Return byte paths if a byte path was given in fsevents (#726) * Pin py.test to a version that supports 2.7 * Disable pypy2 tests on Windows They hang and timeout after 6 hours for some unknown reason * [mac] Add compatibility with old macOS versions (#733) * Ensure version macros are available * Uniformize event for deletion of watched dir (#727) * [inotify] Add support for the IN_CLOSE_WRITE event (#747) * [mac] Support coalesced fsevents (#734) * Add `is_coalesced` property to `NativeEvent` So that we can effectively decide if we need to perform additional system calls to figure out what really happened. * Replace `NativeEvent._event_type` with `repr()` support It's more pythonic, and the `_event_type` implementation wasn't quite usable anyway. NB: the representation is not truly copy/paste python code if there is a double quote inside event.path, but that should be a rare case so we don't add the expensive special case handling there. * Allow running tests with debugger attached Some Python debuggers create additional threads, so we shouldn't assume that there is only one. * Request notifications for watched root * Expect events on macOS instead of using `time.sleep()` It might be even better to check for the emitter class, as opposed to platform * Add exception handling to FSEventsEmitter Reduce the amount of 'silent breakage' * Use sentinel event when setting up tests on macOS So that we can avoid a race between test setup and fseventsd * Improve handling of coalesced events * Revert accidental platform check change * Fix renaming_top_level_directory test on macOS * Generate sub events for move operations * Remove `filesystem_view` again While the `filesystem_view` helps with filtering out additional `FileCreatedEvent`+`DirModifiedEvent` pairs then it also introduces a huge amount of edge cases for synthetic events caused by move and rename operations. On top of that, in order to properly resolve those edge cases we'd have to go back to a solution very similar to the old directory snapshots, with all the performance penalties they suffered from... As such I think it's better to acknowledge the behaviour for coalesced events instead, and thus remove the `filesystem_view` again. * [mac] Improve handling of rename events (#750) * drop support for macOS 10.12 and lower Co-authored-by: SamSchott <ss2151@cam.ac.uk> Co-authored-by: Boris Staletic <boris.staletic@gmail.com> Co-authored-by: Mickaël Schoentgen <contact@tiger-222.fr> Co-authored-by: Dustin Ingram <di@users.noreply.github.com> Co-authored-by: ysard <ysard@users.noreply.github.com> Co-authored-by: Lukas Šupienis <l.supienis@ncs.lt> Co-authored-by: Lukas Šupienis <lukas.supienis@gmail.com>

Yardentexel · 2021-02-11T13:32:38Z

is this implemented only for unix?

BoboTiG · 2021-02-11T15:48:04Z

Yes, IN_CLOSE_WRITE is Unix-only.

Yardentexel · 2021-02-11T15:50:00Z

the answer is yes :)

tried in windows - on_close not working
tried from WSL2 (windows sub system) - on_close not working
tried unix docker image -> works 👍

UnitedMarsupials · 2021-05-08T18:33:50Z

tried unix docker image

You meant Linux image, didn't you? It will not work for other Unixes -- as the kqueue support is not implemented, for example.

glebsts · 2021-10-24T13:45:25Z

Why it was not added to LoggingEventHandler? ... 😿

BoboTiG · 2023-04-04T14:45:53Z

Why it was not added to LoggingEventHandler? ... crying_cat_face

I finally heard you: f3dfc4b :)

You should have open an issue, I didn't catch that until now 😅

Lukas Šupienis and others added 9 commits October 7, 2020 13:11

add: inotify file close events

7c705b3

fix: NOWRITE_CLOSE doesn't modify dir

1adfdd3

update changelog & version

69d61d0

add: test events

2060b52

Revert "update changelog & version"

e1ec07d

This reverts commit 69d61d0.

update changelog

db45bb7

[gnu/linux] Fix ability to catch IN_CLOSE_WRITE, IN_CLOSE_NOWRITE eve…

c66c556

…nts gorakhargosh#690

[gnu/linux] Test IN_CLOSE_WRITE, IN_CLOSE_NOWRITE events

427b5de

Fix typos

c2a9b07

ysard force-pushed the inotify_dev_#690_update branch from 0de211c to c2a9b07 Compare January 13, 2021 01:42

ysard added 2 commits January 13, 2021 19:38

Merge branch 'master' into inotify_dev_#690_update

a32130a

Conflicts: changelog.rst src/watchdog/observers/inotify.py

[gnu/linux] Fix test due to IN_CLOSE_NOWRITE event

b07b1b2

BoboTiG reviewed Jan 14, 2021

View reviewed changes

src/watchdog/events.py Outdated Show resolved Hide resolved

BoboTiG added the inotify label Jan 14, 2021

BoboTiG changed the title ~~Inotify dev #690 update~~ [inotify] Add support for the IN_CLOSE_WRITE event Jan 14, 2021

ysard added a commit to ysard/watchdog that referenced this pull request Jan 14, 2021

[gnu/linux] Revert IN_CLOSE_NOWRITE support gorakhargosh#747

66c1e00

ysard added a commit to ysard/watchdog that referenced this pull request Jan 14, 2021

[gnu/linux] Extend test for IN_CLOSE_WRITE support gorakhargosh#747

dcacde4

ysard added a commit to ysard/watchdog that referenced this pull request Jan 14, 2021

Update changelog gorakhargosh#747, gorakhargosh#690

b282300

BoboTiG reviewed Jan 14, 2021

View reviewed changes

src/watchdog/observers/inotify.py Outdated Show resolved Hide resolved

ysard added 4 commits January 15, 2021 01:05

[gnu/linux] Revert IN_CLOSE_NOWRITE support gorakhargosh#747

9e6d506

[gnu/linux] Extend test for IN_CLOSE_WRITE support gorakhargosh#747

784981d

fix typos

43fd280

Update changelog gorakhargosh#747, gorakhargosh#690

090ab06

ysard force-pushed the inotify_dev_#690_update branch from b282300 to 090ab06 Compare January 15, 2021 00:11

BoboTiG approved these changes Jan 15, 2021

View reviewed changes

BoboTiG merged commit 2fab7c2 into gorakhargosh:master Jan 15, 2021

BoboTiG mentioned this pull request Jan 15, 2021

Implement inotify file close events #690

Closed

BoboTiG mentioned this pull request May 8, 2021

Request: an on_closed-event #791

Closed

BoboTiG mentioned this pull request Jul 18, 2024

Ambiguity in setting FileClosedEvent in filter #1046

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[inotify] Add support for the IN_CLOSE_WRITE event #747

[inotify] Add support for the IN_CLOSE_WRITE event #747

ysard commented Jan 13, 2021

BoboTiG commented Jan 13, 2021 •

edited

Loading

ysard commented Jan 14, 2021

BoboTiG commented Jan 14, 2021

BoboTiG commented Jan 14, 2021 •

edited

Loading

ysard commented Jan 15, 2021

BoboTiG commented Jan 15, 2021

Yardentexel commented Feb 11, 2021

BoboTiG commented Feb 11, 2021

Yardentexel commented Feb 11, 2021

UnitedMarsupials commented May 8, 2021

glebsts commented Oct 24, 2021

BoboTiG commented Apr 4, 2023

[inotify] Add support for the IN_CLOSE_WRITE event #747

[inotify] Add support for the IN_CLOSE_WRITE event #747

Conversation

ysard commented Jan 13, 2021

BoboTiG commented Jan 13, 2021 • edited Loading

ysard commented Jan 14, 2021

BoboTiG commented Jan 14, 2021

BoboTiG commented Jan 14, 2021 • edited Loading

ysard commented Jan 15, 2021

BoboTiG commented Jan 15, 2021

Yardentexel commented Feb 11, 2021

BoboTiG commented Feb 11, 2021

Yardentexel commented Feb 11, 2021

UnitedMarsupials commented May 8, 2021

glebsts commented Oct 24, 2021

BoboTiG commented Apr 4, 2023

BoboTiG commented Jan 13, 2021 •

edited

Loading

BoboTiG commented Jan 14, 2021 •

edited

Loading