Improve handling of rename events on macOS #750

samschott · 2021-01-20T20:56:21Z

At the moment, the two native events which are emitted when an item is renamed are paired to a single MovedEvent by relying on an undocumented behaviour of the File System Events API: two such events will have event IDs which differ only by 1. This may stop working without notice and fails to cover two corner-cases:

When an item is renamed by changing only the casing of the name, the native events appear to have IDs differing by 2.
When a renamed event is coalesced with another event, its event ID may differ from the observed but undocumented behaviour.

This PR matches native event pairs by inode instead. This is achieved by actively recording not only the path but also the inode for all events in the extension module. Such extended information is provided when creating the FSEventsStream with the kFSEventStreamCreateFlagUseExtendedData and kFSEventStreamCreateFlagUseCFTypes flags. This requires dealing CFTypes instead of ctypes in the extension module.

Other changes in this PR:

Expose a method to flush the FSEventsStream.
Simplify the handling of coalesced events in the Python code. This seems to result in more consistent behaviour in case of coalescing.

samschott · 2021-01-20T21:01:53Z

Since this inode information in FSEvents is available only on Mac OSX 10.13 and later, this PR drops support for previous versions. I hope this isn't a blocker since OSX 10.12 is no longer maintained.

BoboTiG · 2021-01-20T21:22:56Z

Thanks @samschott!

1. When an item is renamed by changing only the casing of the name, the native events appear to have IDs differing by 2.

Does that mean you will open a PR to handle that? :)

I am OK to drop macOS 10.12 support 👍

I'll be able to do a review tomorrow (French timezone), could you add a line in the chagelog about the 10.12 support drop? There may be other places where the minimum macOS version is described, but I could do it before the release then.

WDYT @CCP-Aporia?

samschott · 2021-01-20T21:25:35Z

This is the PR that handles that. Case-changes should now be fine, see the added test test_emitter.py:test_case_change.

Yeah, I'll add the information to the change log.

BoboTiG

Minor review.

src/watchdog/observers/fsevents.py

BoboTiG · 2021-01-20T21:29:17Z

Yes, I just saw the test... Sorry :)

samschott · 2021-01-20T21:29:28Z

Also, this is my first time coding anything in C. I think I have eliminated all segfaults and possible issues. But please have a very careful look at the extension module!

Co-authored-by: Mickaël Schoentgen <contact@tiger-222.fr>

BoboTiG · 2021-01-20T21:36:22Z

FTR I plan to move tests to GitHub Actions soon, because Travis-CI is a pain.

…watchdog into improve-rename-handling-macos

CCP-Aporia

First of all, thanks a lot! 👍 This looks great overall. :-)

For the requested changes, please see the inline comments. Basically, increasing the reference count on the created PyObject instances in those places cause a memory leak.

CCP-Aporia · 2021-01-21T08:31:24Z

src/watchdog_fsevents.c

+    CFNumberGetValue(cf_number, kCFNumberSInt64Type, &c_int);
+
+    py_long = PyLong_FromLong(c_int);
+    Py_INCREF(py_long);


PyLong_FromLong returns a new reference, so there's no need to call Py_INCREF here. :-)

Good catch, thanks!

CCP-Aporia · 2021-01-21T08:32:59Z

src/watchdog_fsevents.c

+        py_string = PyUnicode_FromString(c_string_ptr);
+    }
+
+    Py_INCREF(py_string);


PyUnicode_FromString returns a new reference, so there's no need to call Py_INCREF here. :-)

CCP-Aporia · 2021-01-21T08:55:53Z

Just saw that test_observers_api.py:test_event_dispatcher failed on macOS 3.9 on travis. And while it's not related to the changes in this PR, then I believe there's a missing event_dispatcher.join() call at the end of that test case, causing a race condition? 🤔

This reverts commit 75afbd2.

this is the case for kFSEventStreamEventFlagRootChanged

samschott · 2021-01-21T12:20:39Z

Two more changes which I have made:

Reverted the handling of coalesced events. It's off topic for this PR and not as as straight forward as I thought.
Fix a TypeError in the extension module when NativeEvent is passed an inode which is None. This will be the case for events where the kFSEventStreamEventFlagRootChanged flag is set.

CCP-Aporia · 2021-01-21T12:42:41Z

Two more changes which I have made:

Reverted the handling of coalesced events. It's off topic for this PR and not as as straight forward as I thought.

The handling of those events sure does look odd, and I wouldn't be surprised if there's an easier way of expressing what we need to guard there. :-)

Fix a TypeError in the extension module when NativeEvent is passed an inode which is None. This will be the case for events where the kFSEventStreamEventFlagRootChanged flag is set.

Oh, that's an interesting one, there's potential for crashes and memory leaks here. It's necessary to Py_INCREF(self->inode) if parsing was successful. And self->inode needs to be initialised to NULL before the argument parsing happens. Additionally NativeEventObject then needs a tp_dealloc function that does a Py_XDECREF for the inode object.

samschott · 2021-01-21T13:34:43Z

Maybe I should have read the Python/C API docs first... I've hopefully fixed all issues with the inode attribute now 🤞

src/watchdog/observers/fsevents.py

src/watchdog_fsevents.c

tests/test_emitter.py

Co-authored-by: Mickaël Schoentgen <contact@tiger-222.fr>

CCP-Aporia

The C extension looks good to me, so from my point of view this can get merged. 👍

BoboTiG · 2021-01-21T17:20:10Z

Great work @samschott! Thanks for the review @CCP-Aporia 💪

samschott · 2021-01-21T17:42:01Z

Just for future reference, I have tested which events get emitted now when a user edits a file in TextEdit or a similar editor:

NativeEvent(path="Untitled.txt", inode=76814733, flags=10800, id=363703683): is_file, is_renamed
NativeEvent(path="Untitled.txt.sb-c668ce92-TL24mz", inode=76814733, flags=18800, id=363703692): is_file, is_renamed, is_xattr_mod

=> emits:
   FileMovedEvent(src_path="Untitled.txt", dest_path="Untitled.txt.sb-c668ce92-TL24mz")

NativeEvent(path="Untitled.txt", inode=76815055, flags=18c00, id=363703718): is_file, is_inode_meta_mod, is_renamed, is_xattr_mod

=> emits:
   FileCreatedEvent(src_path="Untitled.txt")
   FileModifiedEvent(src_path="Untitled.txt")

NativeEvent(path="Untitled.txt.sb-c668ce92-TL24mz", inode=76814733, flags=41ca00, id=363703735): is_cloned, is_coalesced, is_file, is_owner_change, is_removed, is_renamed, is_xattr_mod

=> emits:
   FileDeletedEvent(src_path="Untitled.txt.sb-c668ce92-TL24mz")

NativeEvent(path="Untitled.txt", inode=76815055, flags=418c00, id=363703799): is_cloned, is_file, is_inode_meta_mod, is_renamed, is_xattr_mod

=> emits:
   FileCreatedEvent(src_path="Untitled.txt")
   FileModifiedEvent(src_path="Untitled.txt")

This is more verbose than the old emitter but seems quite sane to me. Note the is_cloned flag when the file content is copied over from the temp file.

* 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>

Sam Schott added 7 commits January 20, 2021 16:45

drop support for macOS 10.12 and lower

a7696e9

record inodes for all events

2837218

use inode to match rename events

549fdc2

add flush method

d09059b

update tests to new NativeEvent

995458b

simplify handling of created and removed events

75afbd2

added test for case-change

39104cb

BoboTiG reviewed Jan 20, 2021

View reviewed changes

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

clean up loop over native events

7101ffc

Co-authored-by: Mickaël Schoentgen <contact@tiger-222.fr>

Sam Schott added 2 commits January 20, 2021 21:36

added changelog entries

951b9d9

Merge branch 'improve-rename-handling-macos' of github.com:SamSchott/…

0ace028

…watchdog into improve-rename-handling-macos

CCP-Aporia suggested changes Jan 21, 2021

View reviewed changes

Sam Schott added 3 commits January 21, 2021 11:49

remove unneeded Py_INCREF calls

328d1b6

Revert "simplify handling of created and removed events"

22111e8

This reverts commit 75afbd2.

allow inode to be None

85dd926

this is the case for kFSEventStreamEventFlagRootChanged

join event_dispatcher in test_event_dispatcher

8c2e431

Sam Schott added 4 commits January 21, 2021 13:04

fix memory management for inode

7efc7a8

better logs for macOS tests

aa34e70

show flags in log output

364c601

fix returning inode attribute

c9a492e

BoboTiG reviewed Jan 21, 2021

View reviewed changes

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

BoboTiG reviewed Jan 21, 2021

View reviewed changes

src/watchdog_fsevents.c Outdated Show resolved Hide resolved

BoboTiG reviewed Jan 21, 2021

View reviewed changes

tests/test_emitter.py Outdated Show resolved Hide resolved

tweak error message for unsupported macOS

5d86e81

Co-authored-by: Mickaël Schoentgen <contact@tiger-222.fr>

CCP-Aporia approved these changes Jan 21, 2021

View reviewed changes

emit debug instead of info logs from queue_events

36facd6

BoboTiG approved these changes Jan 21, 2021

View reviewed changes

BoboTiG merged commit d9a6e63 into gorakhargosh:master Jan 21, 2021

samschott deleted the improve-rename-handling-macos branch January 21, 2021 17:29

This was referenced Jan 23, 2021

MacOSX: TextEdit event inconsistence #538

Closed

Addresses the issue of out of order events #311

Closed

samschott mentioned this pull request Feb 2, 2021

Watchdog 2.0 samschott/maestral#276

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve handling of rename events on macOS #750

Improve handling of rename events on macOS #750

samschott commented Jan 20, 2021

samschott commented Jan 20, 2021

BoboTiG commented Jan 20, 2021

samschott commented Jan 20, 2021

BoboTiG left a comment

BoboTiG commented Jan 20, 2021

samschott commented Jan 20, 2021

BoboTiG commented Jan 20, 2021 •

edited

Loading

CCP-Aporia left a comment

CCP-Aporia Jan 21, 2021

samschott Jan 21, 2021

CCP-Aporia Jan 21, 2021

CCP-Aporia commented Jan 21, 2021

samschott commented Jan 21, 2021

CCP-Aporia commented Jan 21, 2021

samschott commented Jan 21, 2021 •

edited

Loading

CCP-Aporia left a comment

BoboTiG commented Jan 21, 2021

samschott commented Jan 21, 2021

Improve handling of rename events on macOS #750

Improve handling of rename events on macOS #750

Conversation

samschott commented Jan 20, 2021

samschott commented Jan 20, 2021

BoboTiG commented Jan 20, 2021

samschott commented Jan 20, 2021

BoboTiG left a comment

Choose a reason for hiding this comment

BoboTiG commented Jan 20, 2021

samschott commented Jan 20, 2021

BoboTiG commented Jan 20, 2021 • edited Loading

CCP-Aporia left a comment

Choose a reason for hiding this comment

CCP-Aporia Jan 21, 2021

Choose a reason for hiding this comment

samschott Jan 21, 2021

Choose a reason for hiding this comment

CCP-Aporia Jan 21, 2021

Choose a reason for hiding this comment

CCP-Aporia commented Jan 21, 2021

samschott commented Jan 21, 2021

CCP-Aporia commented Jan 21, 2021

samschott commented Jan 21, 2021 • edited Loading

CCP-Aporia left a comment

Choose a reason for hiding this comment

BoboTiG commented Jan 21, 2021

samschott commented Jan 21, 2021

BoboTiG commented Jan 20, 2021 •

edited

Loading

samschott commented Jan 21, 2021 •

edited

Loading