-
-
Notifications
You must be signed in to change notification settings - Fork 758
Fix DirDeletedEvent not being raised for deleted directories on Windows #1156
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -120,6 +120,28 @@ def _errcheck_dword(value: Any | None, func: Any, args: Any) -> Any: | |
| LPVOID, # FileIOCompletionRoutine # lpCompletionRoutine | ||
| ) | ||
|
|
||
| # ReadDirectoryChangesExW is available on Windows 10 and later. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then you can remove all stuff related to |
||
| # It provides extended information including FileAttributes which allows | ||
| # us to determine if a deleted item was a directory. | ||
| ReadDirectoryChangesExW = kernel32.ReadDirectoryChangesExW | ||
| ReadDirectoryChangesExW.restype = BOOL | ||
| ReadDirectoryChangesExW.errcheck = _errcheck_bool | ||
| ReadDirectoryChangesExW.argtypes = ( | ||
| HANDLE, # hDirectory | ||
| LPVOID, # lpBuffer | ||
| DWORD, # nBufferLength | ||
| BOOL, # bWatchSubtree | ||
| DWORD, # dwNotifyFilter | ||
| ctypes.POINTER(DWORD), # lpBytesReturned | ||
| ctypes.POINTER(OVERLAPPED), # lpOverlapped | ||
| LPVOID, # lpCompletionRoutine | ||
| DWORD, # ReadDirectoryNotifyInformationClass | ||
| ) | ||
|
|
||
| # ReadDirectoryNotifyInformationClass values | ||
| ReadDirectoryNotifyInformation = 1 | ||
| ReadDirectoryNotifyExtendedInformation = 2 | ||
|
|
||
| CreateFileW = kernel32.CreateFileW | ||
| CreateFileW.restype = HANDLE | ||
| CreateFileW.errcheck = _errcheck_handle | ||
|
|
@@ -224,6 +246,33 @@ class FileNotifyInformation(ctypes.Structure): | |
| LPFNI = ctypes.POINTER(FileNotifyInformation) | ||
|
|
||
|
|
||
| # FILE_NOTIFY_EXTENDED_INFORMATION structure for ReadDirectoryChangesExW | ||
| # https://learn.microsoft.com/en-us/windows/win32/api/winnt/ns-winnt-file_notify_extended_information | ||
| class FileNotifyExtendedInformation(ctypes.Structure): | ||
| _fields_ = ( | ||
| ("NextEntryOffset", DWORD), | ||
| ("Action", DWORD), | ||
| ("CreationTime", ctypes.c_longlong), | ||
| ("LastModificationTime", ctypes.c_longlong), | ||
| ("LastChangeTime", ctypes.c_longlong), | ||
| ("LastAccessTime", ctypes.c_longlong), | ||
| ("AllocatedLength", ctypes.c_longlong), | ||
| ("FileSize", ctypes.c_longlong), | ||
| ("FileAttributes", DWORD), | ||
| ("ReparsePointTag", DWORD), | ||
| ("FileId", ctypes.c_longlong), | ||
| ("ParentFileId", ctypes.c_longlong), | ||
| ("FileNameLength", DWORD), | ||
| ("FileName", (ctypes.c_char * 1)), | ||
| ) | ||
|
|
||
|
|
||
| LPFNEI = ctypes.POINTER(FileNotifyExtendedInformation) | ||
|
|
||
| # File attribute constant for directories | ||
| FILE_ATTRIBUTE_DIRECTORY = 0x10 | ||
|
|
||
|
|
||
| # We don't need to recalculate these flags every time a call is made to | ||
| # the win32 API functions. | ||
| WATCHDOG_FILE_FLAGS = FILE_FLAG_BACKUP_SEMANTICS | ||
|
|
@@ -262,15 +311,35 @@ class FileNotifyInformation(ctypes.Structure): | |
| PATH_BUFFER_SIZE = 2048 | ||
|
|
||
|
|
||
| def _parse_event_buffer(read_buffer: bytes) -> list[tuple[int, str]]: | ||
| def _parse_event_buffer(read_buffer: bytes, *, extended: bool = False) -> list[tuple[int, str, bool]]: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need to change the signature. Use extended attrs by default. |
||
| """Parse the event buffer from ReadDirectoryChangesW/ReadDirectoryChangesExW. | ||
|
|
||
| Args: | ||
| read_buffer: The raw buffer from the Windows API call. | ||
| extended: If True, parse as FILE_NOTIFY_EXTENDED_INFORMATION (from ReadDirectoryChangesExW). | ||
| If False, parse as FILE_NOTIFY_INFORMATION (from ReadDirectoryChangesW). | ||
|
|
||
| Returns: | ||
| List of tuples: (action, filename, is_directory) | ||
| When extended=False, is_directory is always False (unknown). | ||
| """ | ||
| n_bytes = len(read_buffer) | ||
| results = [] | ||
| while n_bytes > 0: | ||
| fni = ctypes.cast(read_buffer, LPFNI)[0] # type: ignore[arg-type] | ||
| ptr = ctypes.addressof(fni) + FileNotifyInformation.FileName.offset | ||
| filename = ctypes.string_at(ptr, fni.FileNameLength) | ||
| results.append((fni.Action, filename.decode("utf-16"))) | ||
| num_to_skip = fni.NextEntryOffset | ||
| if extended: | ||
| fnei = ctypes.cast(read_buffer, LPFNEI)[0] # type: ignore[arg-type] | ||
| ptr = ctypes.addressof(fnei) + FileNotifyExtendedInformation.FileName.offset | ||
| filename = ctypes.string_at(ptr, fnei.FileNameLength) | ||
| is_dir = bool(fnei.FileAttributes & FILE_ATTRIBUTE_DIRECTORY) | ||
| results.append((fnei.Action, filename.decode("utf-16"), is_dir)) | ||
| num_to_skip = fnei.NextEntryOffset | ||
| else: | ||
| fni = ctypes.cast(read_buffer, LPFNI)[0] # type: ignore[arg-type] | ||
| ptr = ctypes.addressof(fni) + FileNotifyInformation.FileName.offset | ||
| filename = ctypes.string_at(ptr, fni.FileNameLength) | ||
| # Without extended info, we cannot determine if it was a directory | ||
| results.append((fni.Action, filename.decode("utf-16"), False)) | ||
| num_to_skip = fni.NextEntryOffset | ||
| if num_to_skip <= 0: | ||
| break | ||
| read_buffer = read_buffer[num_to_skip:] | ||
|
|
@@ -329,6 +398,7 @@ def _close_directory_handle(handle: HANDLE) -> None: | |
| class WinAPINativeEvent: | ||
| action: int | ||
| src_path: str | ||
| is_directory: bool = False # Whether the item was a directory (from extended info) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove the comment since the name is pretty explicit. |
||
|
|
||
| @property | ||
| def is_added(self) -> bool: | ||
|
|
@@ -373,13 +443,18 @@ def __init__(self, path: str, *, recursive: bool) -> None: | |
|
|
||
| def _run_inner(self, handle: HANDLE, event_buffer: ctypes.Array[ctypes.c_char], nbytes: DWORD) -> None: | ||
| # This runs in its own thread and it makes a single call to | ||
| # ReadDirectoryChangesW(). This blocks until at least some events are | ||
| # ReadDirectoryChangesExW(). This blocks until at least some events are | ||
| # available (or there is an error). We will re-use the _event_buffer | ||
| # and so we copy the data into a second buffer (double-buffering | ||
| # technique) and queue it for another thread to process. This reduces | ||
| # the time window in which we could miss events. | ||
| # | ||
| # We use ReadDirectoryChangesExW instead of ReadDirectoryChangesW to get | ||
| # extended information including FileAttributes, which allows us to | ||
| # correctly identify deleted directories (see issue #1153). | ||
| # Note: ReadDirectoryChangesExW is only available on Windows 10+. | ||
| try: | ||
| ReadDirectoryChangesW( | ||
| ReadDirectoryChangesExW( | ||
| handle, | ||
| ctypes.byref(event_buffer), | ||
| len(event_buffer), | ||
|
|
@@ -388,6 +463,7 @@ def _run_inner(self, handle: HANDLE, event_buffer: ctypes.Array[ctypes.c_char], | |
| ctypes.byref(nbytes), | ||
| None, | ||
| None, | ||
| ReadDirectoryNotifyExtendedInformation, | ||
| ) | ||
| buf = event_buffer.raw[: nbytes.value] | ||
| except OSError as e: | ||
|
|
@@ -396,7 +472,7 @@ def _run_inner(self, handle: HANDLE, event_buffer: ctypes.Array[ctypes.c_char], | |
| if _is_observed_path_deleted(handle, self._path): | ||
| # Handle the case when the root path is deleted | ||
| with self._lock: | ||
| # Additional calls to ReadDirectoryChangesW() will fail so stop. | ||
| # Additional calls to ReadDirectoryChangesExW() will fail so stop. | ||
| self._should_stop = True | ||
| # Create synthetic event for deletion | ||
| buf = _generate_observed_path_deleted_event() | ||
|
|
@@ -464,5 +540,6 @@ def get_events(self, timeout: float) -> list[WinAPINativeEvent]: | |
| buf = self._buf_queue.get(timeout=timeout) | ||
| except queue.Empty: | ||
| break | ||
| events.extend(_parse_event_buffer(buf)) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since the signature won't change, those edits can be reverted. |
||
| return [WinAPINativeEvent(action, src_path) for action, src_path in events] | ||
| # Use extended=True since we're using ReadDirectoryChangesExW | ||
| events.extend(_parse_event_buffer(buf, extended=True)) | ||
| return [WinAPINativeEvent(action, src_path, is_dir) for action, src_path, is_dir in events] | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,7 +7,7 @@ | |
|
|
||
| import pytest | ||
|
|
||
| from watchdog.events import DirCreatedEvent, DirMovedEvent | ||
| from watchdog.events import DirCreatedEvent, DirDeletedEvent, DirMovedEvent, FileCreatedEvent, FileDeletedEvent | ||
| from watchdog.observers.api import ObservedWatch | ||
| from watchdog.utils import platform | ||
|
|
||
|
|
@@ -127,3 +127,85 @@ def test_root_deleted(event_queue, emitter): | |
|
|
||
| # The emitter is automatically stopped, with no error | ||
| assert not emitter.should_keep_running() | ||
|
|
||
|
|
||
| def test_directory_deleted_event(event_queue, emitter): | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Out of curiosity, existing tests should fail at some point with those changes, can you fix them instead of adding new ones? |
||
| """Test that deleting a directory produces DirDeletedEvent, not FileDeletedEvent. | ||
|
|
||
| This is a regression test for issue #1153 where deleted directories were | ||
| incorrectly reported as FileDeletedEvent because os.path.isdir() cannot | ||
| determine the type of a deleted path. | ||
|
|
||
| The fix uses ReadDirectoryChangesExW which provides FileAttributes in the | ||
| event data, allowing us to correctly identify deleted directories. | ||
| """ | ||
| emitter.start() | ||
| sleep(SLEEP_TIME) | ||
|
|
||
| # Create a directory and a file | ||
| mkdir(p("testdir")) | ||
| sleep(SLEEP_TIME) | ||
|
|
||
| # Delete the directory | ||
| rm(p("testdir"), recursive=True) | ||
| sleep(SLEEP_TIME) | ||
|
|
||
| emitter.stop() | ||
| sleep(SLEEP_TIME) | ||
|
|
||
| got = [] | ||
| while True: | ||
| try: | ||
| event, _ = event_queue.get_nowait() | ||
| except Empty: | ||
| break | ||
| else: | ||
| if event.event_type == "modified": | ||
| # Ignore modified events for deterministic tests | ||
| continue | ||
| got.append(event) | ||
|
|
||
| # Should have DirCreatedEvent and DirDeletedEvent (not FileDeletedEvent) | ||
| assert DirCreatedEvent(p("testdir")) in got | ||
| assert DirDeletedEvent(p("testdir")) in got | ||
| # Make sure we didn't get FileDeletedEvent for the directory | ||
| assert FileDeletedEvent(p("testdir")) not in got | ||
|
|
||
|
|
||
| def test_file_deleted_event(event_queue, emitter): | ||
| """Test that deleting a file still produces FileDeletedEvent. | ||
|
|
||
| This ensures our fix for directory deletion doesn't break file deletion events. | ||
| """ | ||
| from .shell import touch | ||
|
|
||
| emitter.start() | ||
| sleep(SLEEP_TIME) | ||
|
|
||
| # Create a file | ||
| touch(p("testfile.txt")) | ||
| sleep(SLEEP_TIME) | ||
|
|
||
| # Delete the file | ||
| rm(p("testfile.txt")) | ||
| sleep(SLEEP_TIME) | ||
|
|
||
| emitter.stop() | ||
| sleep(SLEEP_TIME) | ||
|
|
||
| got = [] | ||
| while True: | ||
| try: | ||
| event, _ = event_queue.get_nowait() | ||
| except Empty: | ||
| break | ||
| else: | ||
| if event.event_type == "modified": | ||
| continue | ||
| got.append(event) | ||
|
|
||
| # Should have FileCreatedEvent and FileDeletedEvent | ||
| assert FileCreatedEvent(p("testfile.txt")) in got | ||
| assert FileDeletedEvent(p("testfile.txt")) in got | ||
| # Make sure we didn't get DirDeletedEvent for the file | ||
| assert DirDeletedEvent(p("testfile.txt")) not in got | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove the comment.