make RustNotify a context manager and kill the thread on exit
#164
Conversation
Codecov Report
@@ Coverage Diff @@
## main #164 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 6 6
Lines 375 375
Branches 79 79
=========================================
Hits 375 375
Continue to review full report at Codecov.
|
| } | ||
|
|
||
| pub fn close(&mut self) { | ||
| self.watcher = WatcherEnum::None; |
There was a problem hiding this comment.
Can you override __del__ to issue a ResourceWarning if the watcher is not None?
There was a problem hiding this comment.
Maybe, but I'm not sure it's worth it - if you actually delete the instance the thread will be closed. So technically __del__ is just as correct as __exit__ or close.
Also RustNotify is not supposed to be used directly and both the public methods which use take care of clean up.
I think what we have here is sufficient.
There was a problem hiding this comment.
- if you actually delete the instance the thread will be closed.
Yes this is the purpose of ResourceWarning in a __del__ method
There was a problem hiding this comment.
pytest hooks into this GC/ResourceWarning system to fail the test suite if an explicit close is forgotten
There was a problem hiding this comment.
But omitting to call close is fine in general.
If you'd like to submit a PR, feel free.
There was a problem hiding this comment.
Apart from your warning suggestion on __del__, can you confirm if this works?
There was a problem hiding this comment.
yep this fixes it!
/tmp/tmphp91terl
watcher: INotifyWatcher { channel: Sender { .. }, waker: Waker { inner: Waker { fd: File { fd: 5, path: "anon_inode:[eventfd]", read: true, write: true } } } }
keyboard interrupt happened gen=<generator object watch at 0x7f2abdb68a50> is closed gen.gi_frame=None is unset but still we're listening
There was a problem hiding this comment.
On reflection I think you're right that we should raise a resource warning, fuller answer below.
There was a problem hiding this comment.
I've created #166 to keep track of adding a resource warning. I think htis can be merged through since we shouldn't wait for pyo3 to resolve the original bug here.
fix #163
@graingert this is what I meant. Could you test and check that this is closing the thread on exit?