From 62e78fbbfcb5724994f4821c9aba1812c9d33da8 Mon Sep 17 00:00:00 2001 From: Samuel Colvin Date: Thu, 23 Jun 2022 00:17:32 +0100 Subject: [PATCH 1/5] make RustNotify a context manager and kill the thread on exit --- src/lib.rs | 19 +++++++- tests/conftest.py | 9 ++++ tests/test_watch.py | 9 ++++ watchfiles/_rust_notify.pyi | 14 +++++- watchfiles/main.py | 96 ++++++++++++++++++------------------- 5 files changed, 97 insertions(+), 50 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 6320ba31..7b9fe5c7 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -29,6 +29,7 @@ const CHANGE_DELETED: u8 = 3; #[derive(Debug)] enum WatcherEnum { + None, Poll(PollWatcher), Recommended(RecommendedWatcher), } @@ -157,13 +158,16 @@ impl RustNotify { timeout_ms: u64, stop_event: PyObject, ) -> PyResult { + if matches!(self.watcher, WatcherEnum::None) { + return Err(PyRuntimeError::new_err("Watcher already closed")); + } let stop_event_is_set: Option<&PyAny> = match stop_event.is_none(py) { true => None, false => { let event: &PyAny = stop_event.extract(py)?; let func: &PyAny = event.getattr("is_set")?.extract()?; if !func.is_callable() { - return Err(PyTypeError::new_err("'stop_event.is_set' must be callable".to_string())); + return Err(PyTypeError::new_err("'stop_event.is_set' must be callable")); } Some(func) } @@ -228,6 +232,19 @@ impl RustNotify { Ok(py_changes) } + // https://github.com/PyO3/pyo3/issues/1205#issuecomment-778529199 + pub fn __enter__(slf: PyRef) -> PyRef { + slf + } + + pub fn close(&mut self) { + self.watcher = WatcherEnum::None; + } + + pub fn __exit__(&mut self, _exc_type: PyObject, _exc_value: PyObject, _traceback: PyObject) { + self.close(); + } + pub fn __repr__(&self) -> PyResult { Ok(format!("RustNotify({:#?})", self.watcher)) } diff --git a/tests/conftest.py b/tests/conftest.py index 696ae567..a6370c21 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -79,6 +79,15 @@ def watch(self, debounce_ms: int, step_ms: int, timeout_ms: int, cancel_event): self.watch_count += 1 return change + def __enter__(self): + return self + + def __exit__(self, *args): + self.close() + + def close(self): + pass + if TYPE_CHECKING: from typing import Literal, Protocol diff --git a/tests/test_watch.py b/tests/test_watch.py index f6da3f5b..33e804f4 100644 --- a/tests/test_watch.py +++ b/tests/test_watch.py @@ -201,6 +201,15 @@ def watch(self, *args): self.i += 1 return {(Change.added, 'spam.py')} + def __enter__(self): + return self + + def __exit__(self, *args): + self.close() + + def close(self): + pass + async def test_awatch_interrupt_raise(mocker, caplog): mocker.patch('watchfiles.main.RustNotify', return_value=MockRustNotifyRaise()) diff --git a/watchfiles/_rust_notify.pyi b/watchfiles/_rust_notify.pyi index 911f3d05..29354508 100644 --- a/watchfiles/_rust_notify.pyi +++ b/watchfiles/_rust_notify.pyi @@ -1,4 +1,4 @@ -from typing import List, Literal, Optional, Protocol, Set, Tuple, Union +from typing import Any, List, Literal, Optional, Protocol, Set, Tuple, Union __all__ = 'RustNotify', 'WatchfilesRustInternalError' @@ -56,6 +56,18 @@ class RustNotify: `'signal'` if a signal was received, `'stop'` if the `stop_event` was set, or `'timeout'` if `timeout_ms` was exceeded. """ + def __enter__(self) -> 'RustNotify': + """ + Does nothing, but allows `RustNotify` to be used as a context manager. + """ + def __exit__(self, *args: Any) -> None: + """ + Calls close. + """ + def close(self) -> None: + """ + Stop the watching thread. + """ class WatchfilesRustInternalError(RuntimeError): """ diff --git a/watchfiles/main.py b/watchfiles/main.py index 5bc144d1..277ec245 100644 --- a/watchfiles/main.py +++ b/watchfiles/main.py @@ -100,27 +100,27 @@ def watch( print(changes) ``` """ - watcher = RustNotify([str(p) for p in paths], debug, force_polling, poll_delay_ms) - while True: - raw_changes = watcher.watch(debounce, step, rust_timeout, stop_event) - if raw_changes == 'timeout': - if yield_on_timeout: - yield set() - else: - logger.debug('rust notify timeout, continuing') - elif raw_changes == 'signal': - if raise_interrupt: - raise KeyboardInterrupt - else: - logger.warning('KeyboardInterrupt caught, stopping watch') + with RustNotify([str(p) for p in paths], debug, force_polling, poll_delay_ms) as watcher: + while True: + raw_changes = watcher.watch(debounce, step, rust_timeout, stop_event) + if raw_changes == 'timeout': + if yield_on_timeout: + yield set() + else: + logger.debug('rust notify timeout, continuing') + elif raw_changes == 'signal': + if raise_interrupt: + raise KeyboardInterrupt + else: + logger.warning('KeyboardInterrupt caught, stopping watch') + return + elif raw_changes == 'stop': return - elif raw_changes == 'stop': - return - else: - changes = _prep_changes(raw_changes, watch_filter) - if changes: - _log_changes(changes) - yield changes + else: + changes = _prep_changes(raw_changes, watch_filter) + if changes: + _log_changes(changes) + yield changes async def awatch( # noqa C901 @@ -214,35 +214,35 @@ async def stop_soon(): else: stop_event_ = stop_event - watcher = RustNotify([str(p) for p in paths], debug, force_polling, poll_delay_ms) - timeout = _calc_async_timeout(rust_timeout) - CancelledError = anyio.get_cancelled_exc_class() - - while True: - async with anyio.create_task_group() as tg: - try: - raw_changes = await anyio.to_thread.run_sync(watcher.watch, debounce, step, timeout, stop_event_) - except (CancelledError, KeyboardInterrupt): - stop_event_.set() - # suppressing KeyboardInterrupt wouldn't stop it getting raised by the top level asyncio.run call - raise - tg.cancel_scope.cancel() - - if raw_changes == 'timeout': - if yield_on_timeout: - yield set() + with RustNotify([str(p) for p in paths], debug, force_polling, poll_delay_ms) as watcher: + timeout = _calc_async_timeout(rust_timeout) + CancelledError = anyio.get_cancelled_exc_class() + + while True: + async with anyio.create_task_group() as tg: + try: + raw_changes = await anyio.to_thread.run_sync(watcher.watch, debounce, step, timeout, stop_event_) + except (CancelledError, KeyboardInterrupt): + stop_event_.set() + # suppressing KeyboardInterrupt wouldn't stop it getting raised by the top level asyncio.run call + raise + tg.cancel_scope.cancel() + + if raw_changes == 'timeout': + if yield_on_timeout: + yield set() + else: + logger.debug('rust notify timeout, continuing') + elif raw_changes == 'stop': + return + elif raw_changes == 'signal': + # in theory the watch thread should never get a signal + raise RuntimeError('watch thread unexpectedly received a signal') else: - logger.debug('rust notify timeout, continuing') - elif raw_changes == 'stop': - return - elif raw_changes == 'signal': - # in theory the watch thread should never get a signal - raise RuntimeError('watch thread unexpectedly received a signal') - else: - changes = _prep_changes(raw_changes, watch_filter) - if changes: - _log_changes(changes) - yield changes + changes = _prep_changes(raw_changes, watch_filter) + if changes: + _log_changes(changes) + yield changes def _prep_changes( From 56114aac9fc5d126bbf9b5a9e127a5c62fb2101e Mon Sep 17 00:00:00 2001 From: Samuel Colvin Date: Thu, 23 Jun 2022 00:26:44 +0100 Subject: [PATCH 2/5] test for closed watcher --- src/lib.rs | 2 +- tests/test_rust_notify.py | 8 +++++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 7b9fe5c7..4c0d2a33 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -159,7 +159,7 @@ impl RustNotify { stop_event: PyObject, ) -> PyResult { if matches!(self.watcher, WatcherEnum::None) { - return Err(PyRuntimeError::new_err("Watcher already closed")); + return Err(PyRuntimeError::new_err("RustNotify watcher closed")); } let stop_event_is_set: Option<&PyAny> = match stop_event.is_none(py) { true => None, diff --git a/tests/test_rust_notify.py b/tests/test_rust_notify.py index c075607a..775b4572 100644 --- a/tests/test_rust_notify.py +++ b/tests/test_rust_notify.py @@ -16,10 +16,16 @@ def test_add(test_dir: Path): assert watcher.watch(200, 50, 500, None) == {(1, str(test_dir / 'new_file.txt'))} -def test_recommended_repr(test_dir: Path): +def test_close(test_dir: Path): watcher = RustNotify([str(test_dir)], True, False, 0) assert repr(watcher).startswith('RustNotify(Recommended(\n') + watcher.close() + + assert repr(watcher) == 'RustNotify(None)' + with pytest.raises(RuntimeError, match='RustNotify watcher closed'): + watcher.watch(200, 50, 500, None) + def test_modify_write(test_dir: Path): watcher = RustNotify([str(test_dir)], True, False, 0) From 843d6a984dfbe06b2ad90fe8248a60d0210fff80 Mon Sep 17 00:00:00 2001 From: Samuel Colvin Date: Thu, 23 Jun 2022 09:39:46 +0100 Subject: [PATCH 3/5] use Py not PyRef --- src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib.rs b/src/lib.rs index 4c0d2a33..a09a6e6f 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -233,7 +233,7 @@ impl RustNotify { } // https://github.com/PyO3/pyo3/issues/1205#issuecomment-778529199 - pub fn __enter__(slf: PyRef) -> PyRef { + pub fn __enter__(slf: Py) -> Py { slf } From ce2e11711acdd2d22b54257afa0099d21ebd75fc Mon Sep 17 00:00:00 2001 From: Samuel Colvin Date: Thu, 23 Jun 2022 09:53:44 +0100 Subject: [PATCH 4/5] update docs --- docs/api/rust_backend.md | 13 +++++++++++++ src/lib.rs | 2 +- watchfiles/_rust_notify.pyi | 16 ++++++++++++++-- 3 files changed, 28 insertions(+), 3 deletions(-) diff --git a/docs/api/rust_backend.md b/docs/api/rust_backend.md index 938e1af1..9547ca14 100644 --- a/docs/api/rust_backend.md +++ b/docs/api/rust_backend.md @@ -15,3 +15,16 @@ r = RustNotify(['first/path', 'second/path'], False, False, 0) changes = r.watch(1_600, 50, 100, None) print(changes) ``` + +Or using `RustNotify` as a context manager: + +```py +title="Rust backend context manager example" +from watchfiles._rust_notify import RustNotify + +with RustNotify(['first/path', 'second/path'], False, False, 0) as r: + changes = r.watch(1_600, 50, 100, None) + print(changes) +``` + +(See the documentation on `close` above for when the context manager or `close` method are required.) diff --git a/src/lib.rs b/src/lib.rs index a09a6e6f..e3c6543e 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -232,7 +232,7 @@ impl RustNotify { Ok(py_changes) } - // https://github.com/PyO3/pyo3/issues/1205#issuecomment-778529199 + /// https://github.com/PyO3/pyo3/issues/1205#issuecomment-1164096251 for advice on `__enter__` pub fn __enter__(slf: Py) -> Py { slf } diff --git a/watchfiles/_rust_notify.pyi b/watchfiles/_rust_notify.pyi index 29354508..4cd5d6eb 100644 --- a/watchfiles/_rust_notify.pyi +++ b/watchfiles/_rust_notify.pyi @@ -10,7 +10,7 @@ class AbstractEvent(Protocol): class RustNotify: """ Interface to the Rust [notify](https://crates.io/crates/notify) crate which does - the heavy lifting of watching for file changes and grouping them into a single event. + the heavy lifting of watching for file changes and grouping them into events. """ def __init__(self, watch_paths: List[str], debug: bool, force_polling: bool, poll_delay_ms: int) -> None: @@ -59,6 +59,9 @@ class RustNotify: def __enter__(self) -> 'RustNotify': """ Does nothing, but allows `RustNotify` to be used as a context manager. + + Note: the watching thead is created when an instance is initiated, not on + `__enter__`. """ def __exit__(self, *args: Any) -> None: """ @@ -66,7 +69,16 @@ class RustNotify: """ def close(self) -> None: """ - Stop the watching thread. + Stops the watching thread. After `close` is called, the RustNotify instance can no + longer be used, calls to [`watch`][watchfiles.RustNotify.watch] will raise a `RuntimeError`. + + Note: `close` is not required, just deleting the `RustNotify` instance will kill the thread + implicitly. + + As per samuelcolvin/watchfiles#163 `close()` is only required because in the + event of an error, the traceback in `sys.exc_info` keeps a reference to `watchfiles.watch`'s + frame, so you can't rely on the `RustNotify` object being deleted, and thereby stopping + the watching thread. """ class WatchfilesRustInternalError(RuntimeError): From fee586362235547bc0ba7ff7792e0bec19a6ac2e Mon Sep 17 00:00:00 2001 From: Samuel Colvin Date: Sat, 25 Jun 2022 12:15:36 +0100 Subject: [PATCH 5/5] make clear private --- src/lib.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/lib.rs b/src/lib.rs index e3c6543e..f81c7876 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -248,7 +248,9 @@ impl RustNotify { pub fn __repr__(&self) -> PyResult { Ok(format!("RustNotify({:#?})", self.watcher)) } +} +impl RustNotify { fn clear(&self) { self.changes.lock().unwrap().clear(); }