Repository: gorakhargosh/watchdog
Pull request #727 (2020-12-10 19:49:40)

Fixes missing notification for deletion of watched dir on macOS
This PR fixes an issue which would prevent us from being notified when the watched directory is deleted. This is achieved by passing the `kFSEventStreamCreateFlagWatchRoot` to the watch.

CCP-Aporia: Thanks for this, but there appear to be a few gotchas with `kFSEventStreamCreateFlagWatchRoot` as far as I can see: 

According to [Apple's file system events documentation](https://developer.apple.com/library/archive/documentation/Darwin/Conceptual/FSEvents_ProgGuide/UsingtheFSEventsFramework/UsingtheFSEventsFramework.html#//apple_ref/doc/uid/TP40005289-CH4-SW4) then `RootChanged` indicates more than just a deletion. 
Also note that it resets the event ID to 0 in case of a move, so there might be more work required in the `FSEventsEmitter` to not break.

I'll take a look tomorrow to whip up a few test cases to see what happens (on Big Sur at least, no access to older versions here).
SamSchott: Indeed, `RootChanged` will be emitted if the watched folder is moved and not deleted. A proper deletion is emitted when the watched folder is deleted (only with `kFSEventStreamCreateFlagWatchRoot` enabled, though). Keeping with the watchdog API, which does allow for a watch to be dynamically moved to a new folder, I thought it best to treat `RootChanged` as a deleted event (like any other rename to a destination outside of `watch.path`).

Also, the event ID is not "reset". Only the `RootChanged` event itself will have an ID of zero, all other file system events will continue to have global and continuously increasing event IDs.
BoboTiG: Thank a lot @SamSchott !

I will wait for @CCP-Aporia approval. In the meantime, could you rebase your PR 🙏 ?
CCP-Aporia: If the public API should simply be that a changed root always translates into a `DirDeleted` event, then this is certainly fine and just misses a few tests (I'm currently working on some). 🙂 

SamSchott: Rebased. @BoboTiG, could you confirm what the expected behaviour is for a moved root? I was merely extrapolating from the behaviour when child paths are moved out of root.
SamSchott: @CCP-Aporia, there is already a test for the root being deleted (which is failing without this PR) but no test currently for the root being renamed.
BoboTiG: I think we need a unified behavior.
On Windows: https://github.com/gorakhargosh/watchdog/blob/44b9618f147bb74bbfab382964d4e6ae674e1a15/src/watchdog/observers/read_directory_changes.py#L123-L124
On Linux: https://github.com/gorakhargosh/watchdog/blob/44b9618f147bb74bbfab382964d4e6ae674e1a15/src/watchdog/observers/inotify_c.py#L310-L314 and https://github.com/gorakhargosh/watchdog/blob/44b9618f147bb74bbfab382964d4e6ae674e1a15/src/watchdog/observers/inotify_c.py#L521 is never used.

So on other OSes, we just skip the event and even stop the observer on Windows.

Maybe this is not ideal 🤔 

Should we just fire a `DirDeletedEvent` and stop the observer?
BoboTiG: For a moved root, I would say this is the same behavior. It should just fire the `DirMovedEvent`event and stop the observer. But I rally need inputs because this may not be the better idea.
SamSchott: I think it's definitely good to fire a `DirDeletedEvent` before stopping the observer when the root is deleted. Not so sure about  moving, a `DirMovedEvent` may be more appropriate because it's more informative.
SamSchott: Always firing a `DirDeletedEvent` will be easier to implement because finding out where the directory moved to requires some more work, at least on macOS. From their docs [here](https://developer.apple.com/library/archive/documentation/Darwin/Conceptual/FSEvents_ProgGuide/UsingtheFSEventsFramework/UsingtheFSEventsFramework.html#//apple_ref/doc/uid/TP40005289-CH4-SW4):

> If you need to figure out where the directory moved, you should open the root directory with open, then pass F_GETPATH to fcntl to find its current path. See the manual page for fcntl for more information.



CCP-Aporia: Agreeing with @SamSchott, and I'd even go as far as updating the watched path in the observer in case of a `DirMovedEvent`.
BoboTiG: We can start light with only targeting the deleted root in that PR. Then, as the rename/move root is more work, it would be good to tackle it in a separate branch.

@SamSchott can you ensure the behavior will be the same on all implementation?
SamSchott: Will do. I have expanded the tests to check for a `DirDeletedEvent` on all platforms now and check that the emitter thread is stopped. I have also adapted the Windows emitter to actually emit a `DirDeletedEvent`.

From looking at the code, it seems to me that the inotify backend already emits a `DirDeletedEvent`:

https://github.com/gorakhargosh/watchdog/blob/44b9618f147bb74bbfab382964d4e6ae674e1a15/src/watchdog/observers/inotify_buffer.py#L93-L98

Lets see what the tests give...


SamSchott: There was a remaining issue with propagating the deleted event in inotify. Apart from this, `test_delete_self` should be passing on all platforms. 

However, there remain issues with moving the root folder which will be difficult to solve. This is why I have removed `test_move_self` for now and would propose to tackle this in a separate PR. In particular:

1. On Windows, the watch is created the a handle to the directory and not the path. When deleting the underlying directory, this raises an error which is caught in `winapi.py` and used to generate the deleted event:
   https://github.com/gorakhargosh/watchdog/blob/44b9618f147bb74bbfab382964d4e6ae674e1a15/src/watchdog/observers/winapi.py#L343-L349
  
    However, the watch seemingly happily continues when the root directory is moved without emitting *any* event. This follows the documented behaviour [here](https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-readdirectorychangesw). I do not have access to a Windows machine so cannot test if this is also the case when moving the root to trash.

2. On inotify, we also don't receive a native event when the root is moved. This is problematic because it *does* inlcude moving the root to the trash (which is how end users typically delete a folder...). Fortunately, we can change this easily by requesting to be notified for `IN_MOVE_SELF` events.

3. On macOS there is the known "problem" of being notified that the root changed but not knowing if it was deleted (`os.path.exists` should do the trick here) or moved and where it was moved to. As previously quoted, Apple docs recommend opening the root with open and passing `F_GETPATH` to `fcntl` to find its new path. This should be easy enough apart from Python not exposing `F_GETPATH` (see [here](https://bugs.python.org/issue37964)). It's probably best to do this in the extension module instead.

To summaries, there are clear paths forward on FSEvents and Inotify and possibly a way on Windows. However, it involves moderate amounts of work and a choice of API: Do we want to emit a `DirDeletedEvent` or a `DirMovedEvent`? In case of the latter, do we want do move the watch or cancel it? To stay consistent with the API when an item is moved outside the root path, I would probably favour just emitted a `DirDeletedEvent` and leaving it at that. But of course it's my choice!

In any case, dealing with a moved root is probably the topic of a separate PR.

Patch:
diff --git a/changelog.rst b/changelog.rst
index 1f21910..f052c9d 100644
--- a/changelog.rst
+++ b/changelog.rst
@@ -9,7 +9,8 @@ Changelog
 202x-xx-xx • `full history <https://github.com/gorakhargosh/watchdog/compare/v1.0.1...master>`__
 
 - [mac] Fix missing ``event_id`` attribute in ``fsevents`` (`#721 <https://github.com/gorakhargosh/watchdog/pull/721>`_)
-- Thanks to our beloved contributors: @SamSchott
+- [mac] Add compatibility with old macOS versions (`#733` <https://github.com/gorakhargosh/watchdog/pull/733>`_)
+- Thanks to our beloved contributors: @SamSchott, @CCP-Aporia
 
 
 1.0.1
diff --git a/src/watchdog/observers/fsevents.py b/src/watchdog/observers/fsevents.py
index 1226ca4..ecfdcee 100644
--- a/src/watchdog/observers/fsevents.py
+++ b/src/watchdog/observers/fsevents.py
@@ -123,6 +123,19 @@ def queue_events(self, timeout):
                     cls = DirDeletedEvent if event.is_directory else FileDeletedEvent
                     self.queue_event(cls(src_path))
                     self.queue_event(DirModifiedEvent(os.path.dirname(src_path)))
+
+                    if src_path == self.watch.path:
+                        # this should not really occur, instead we expect
+                        # is_root_changed to be set
+                        self.stop()
+
+                elif event.is_root_changed:
+                    # This will be set if root or any if its parents is renamed or
+                    # deleted.
+                    # TODO: find out new path and generate DirMovedEvent?
+                    self.queue_event(DirDeletedEvent(self.watch.path))
+                    self.stop()
+
                 i += 1
 
     def run(self):
diff --git a/src/watchdog/observers/inotify.py b/src/watchdog/observers/inotify.py
index f814fa9..5cb8c5d 100644
--- a/src/watchdog/observers/inotify.py
+++ b/src/watchdog/observers/inotify.py
@@ -170,6 +170,9 @@ def queue_events(self, timeout, full_events=False):
                 cls = DirCreatedEvent if event.is_directory else FileCreatedEvent
                 self.queue_event(cls(src_path))
                 self.queue_event(DirModifiedEvent(os.path.dirname(src_path)))
+            elif event.is_delete_self and src_path == self.watch.path:
+                self.queue_event(DirDeletedEvent(src_path))
+                self.stop()
 
     def _decode_path(self, path):
         """Decode path only if unicode string was passed to this emitter. """
diff --git a/src/watchdog/observers/read_directory_changes.py b/src/watchdog/observers/read_directory_changes.py
index 5555630..a0e088e 100644
--- a/src/watchdog/observers/read_directory_changes.py
+++ b/src/watchdog/observers/read_directory_changes.py
@@ -22,6 +22,7 @@
 
 from watchdog.events import (
     DirCreatedEvent,
+    DirDeletedEvent,
     DirMovedEvent,
     DirModifiedEvent,
     FileCreatedEvent,
@@ -121,6 +122,7 @@ def queue_events(self, timeout):
                 elif winapi_event.is_removed:
                     self.queue_event(FileDeletedEvent(src_path))
                 elif winapi_event.is_removed_self:
+                    self.queue_event(DirDeletedEvent(self.watch.path))
                     self.stop()
 
 
diff --git a/src/watchdog_fsevents.c b/src/watchdog_fsevents.c
index a8e8b98..bbda66a 100644
--- a/src/watchdog_fsevents.c
+++ b/src/watchdog_fsevents.c
@@ -20,12 +20,38 @@
 
 
 #include <Python.h>
+#include <Availability.h>
 #include <CoreFoundation/CoreFoundation.h>
 #include <CoreServices/CoreServices.h>
 #include <stdlib.h>
 #include <signal.h>
 
 
+/* Compatibility; since fsevents won't set these on earlier macOS versions the properties will always be False */
+#if MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_7
+#error Watchdog module requires at least Mac OS X 10.7
+#endif
+#ifndef MAC_OS_X_VERSION_10_9
+#define MAC_OS_X_VERSION_10_9         1090
+#endif
+#ifndef MAC_OS_X_VERSION_10_10
+#define MAC_OS_X_VERSION_10_10      101000
+#endif
+#ifndef MAC_OS_X_VERSION_10_13
+#define MAC_OS_X_VERSION_10_13      101300
+#endif
+#if MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_9
+#define kFSEventStreamEventFlagOwnEvent 0x00080000
+#endif
+#if MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_10
+#define kFSEventStreamEventFlagItemIsHardlink 0x00100000
+#define kFSEventStreamEventFlagItemIsLastHardlink 0x00200000
+#endif
+#if MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_13
+#define kFSEventStreamEventFlagItemCloned 0x00400000
+#endif
+
+
 /* Convenience macros to make code more readable. */
 #define G_NOT(o)                        !o
 #define G_IS_NULL(o)                    o == NULL
@@ -467,7 +493,7 @@ watchdog_FSEventStreamCreate(StreamCallbackInfo *stream_callback_info_ref,
                                      paths,
                                      kFSEventStreamEventIdSinceNow,
                                      stream_latency,
-                                     kFSEventStreamCreateFlagNoDefer | kFSEventStreamCreateFlagFileEvents);
+                                     kFSEventStreamCreateFlagNoDefer | kFSEventStreamCreateFlagFileEvents | kFSEventStreamCreateFlagWatchRoot);
     CFRelease(paths);
     return stream_ref;
 }


Test patch:
diff --git a/tests/test_emitter.py b/tests/test_emitter.py
index 082ec38..6df98ae 100644
--- a/tests/test_emitter.py
+++ b/tests/test_emitter.py
@@ -64,7 +64,11 @@ def setup_teardown(tmpdir):
 
     yield
 
-    emitter.stop()
+    try:
+        emitter.stop()
+    except OSError:
+        # watch was already stopped, e.g., in `test_delete_self`
+        pass
     emitter.join(5)
     assert not emitter.is_alive()
 
@@ -283,10 +287,12 @@ def test_delete_self():
     start_watching(p('dir1'))
     rm(p('dir1'), True)
 
-    if platform.is_darwin():
-        event = event_queue.get(timeout=5)[0]
-        assert event.src_path == p('dir1')
-        assert isinstance(event, FileDeletedEvent)
+    event = event_queue.get(timeout=5)[0]
+    assert event.src_path == p('dir1')
+    assert isinstance(event, DirDeletedEvent)
+
+    emitter.join(timeout=1)
+    assert not emitter.is_alive()
 
 
 @pytest.mark.skipif(platform.is_windows() or platform.is_bsd(),
diff --git a/tests/test_fsevents.py b/tests/test_fsevents.py
index bdbf35a..88a42e6 100644
--- a/tests/test_fsevents.py
+++ b/tests/test_fsevents.py
@@ -100,7 +100,9 @@ def on_thread_stop(self):
     w = observer.schedule(FileSystemEventHandler(), a, recursive=False)
     rmdir(a)
     time.sleep(0.1)
-    observer.unschedule(w)
+    with pytest.raises(KeyError):
+        # watch no longer exists!
+        observer.unschedule(w)
 
 
 def test_watchdog_recursive():
diff --git a/tests/test_inotify_c.py b/tests/test_inotify_c.py
index 81f6586..a18f140 100644
--- a/tests/test_inotify_c.py
+++ b/tests/test_inotify_c.py
@@ -40,7 +40,11 @@ def watching(path=None, use_full_emitter=False):
     emitter = Emitter(event_queue, ObservedWatch(path, recursive=True))
     emitter.start()
     yield
-    emitter.stop()
+    try:
+        emitter.stop()
+    except OSError:
+        # watch was already stopped, e.g., because root was deleted
+        pass
     emitter.join(5)