Fix race condition in once #77
Conversation
There was a problem hiding this comment.
This loops pretty good! I think beyond stylistic things what I'd like to see implementation-wise is the lock used in the other non-removal spots where things are mucked around with, particularly in _add_event_handler and _call_handlers.
It would also be great if we can add a test for the sync handler, something like:
ee = EventEmitter()
@ee.once("event")
def remove_self():
ee.remove_listener("event", remove_self)
ee.emit("event")This should crash before this change but work afterwards.
Thanks for taking a look at this! I really appreciate it, it's saving me a lot of work.
pyee/_base.py
Outdated
| @@ -38,6 +39,7 @@ def on_error(message): | |||
| """ | |||
| def __init__(self): | |||
| self._events = defaultdict(OrderedDict) | |||
| self.lock = Lock() | |||
There was a problem hiding this comment.
Changed it to _lock
|
Good comments all around will try to get an update done tonight / tomorrow |
1 similar comment
|
Good comments all around will try to get an update done tonight / tomorrow |
|
Cool! Any work you do on this is much appreciated, and if you run out of steam I can always take and run with your work. |
|
The testcase became a bit more convoluted since the removal is done before the target function is called (I guess to minimize the risk of this kind of race). Let me know if the solution is too hacky. |
| if event in self._events and f in self._events[event]: | ||
| self._remove_listener(event, f) | ||
| else: | ||
| return None |
There was a problem hiding this comment.
Yeah OK I see what you mean here. Interesting, I wouldn't have expected the edge case we saw given this ordering (though in a threaded context without locks it makes sense). Guess I should have looked more closely!
Mulling this over, I think shrugging and returning None is OK, because if the edge case we're talking about triggers it probably means that the handler has either already been called or was intentionally removed between the function executing and the lock yielding.
tests/test_sync.py
Outdated
| @@ -203,6 +204,21 @@ def once_handler(data): | |||
| assert ee._events['event'] == OrderedDict() | |||
|
|
|||
|
|
|||
| def test_once_remove_race(): | |||
There was a problem hiding this comment.
Oof, yeah, OK, I see what you mean with this test. I wish there was a sensible way to test this, but I think that given what we have here it's too hairy to implement what we had in mind. My vote is to remove this test and shrug.
Add check that listener method is still registered before removing. To ensure atomic execution a lock is added around removals of listeners.
forslund
force-pushed
the
locked-removal
branch
from
2931b24
to
156af60
Compare
|
I dropped the test commit from the branch and squashed the remaining two commits into a single one. should be good to go now. |
|
Released in 8.1.0. Thanks! |
This is a naive locking approach to the issue reported in #76
This PR adds a check in the once-wrapper that listener method is still registered before removing. To ensure atomic execution a lock is added around removals of listeners.