Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3518 +/- ##
===============================================
Coverage 100.00000% 100.00000%
===============================================
Files 128 128
Lines 19471 19498 +27
Branches 1323 1324 +1
===============================================
+ Hits 19471 19498 +27
🚀 New features to boost your workflow:
|
A5rocks
left a comment
There was a problem hiding this comment.
Thanks! I was hoping we could add some extra state to ensure that the only events we must skip are these. An initial thing I was thinking about was e.g. add the relevant key to a list when being cancelled, clear at the start of process_events, and ensure anything we skip is in that list.
Does that make sense? Maybe there's something I'm missing, or maybe it takes too many lines to implement for what should never happen...
| self._force_wakeup.drain() | ||
| continue | ||
| receiver = self._registered[key] | ||
| receiver = self._registered.get(key) |
There was a problem hiding this comment.
Since this is non-obvious, could you cite the test here? e.g. just say # see <test name>. (alternatively, implementing my suggestion would also be enough explanation, I think)
There was a problem hiding this comment.
Thanks — followed up in 6e9ce05. process_events() now only skips keys that were actually removed by cancellation for that batch, and an unknown-key regression test verifies unrelated missing registrations still raise. The CI run on that head is green.
|
Thanks, that makes sense. I’ve updated the follow-up so cancellation records only the key whose unregister actually succeeded; |
|
Follow-up is in: cancellation now records only keys whose abort actually succeeded, and |
| check(expected_monitors=0, expected_readers=1, expected_writers=0) | ||
|
|
||
|
|
||
| if sys.platform not in {"win32", "linux"} and not TYPE_CHECKING: |
There was a problem hiding this comment.
Why not do pytest.mark.skipif here?
There was a problem hiding this comment.
Good point. The earlier skipif version tripped the Linux mypy pass on the kqueue-only names. I restored skipif in 55753d8 and used the existing attr-defined/unused-ignore pattern for those platform-specific attributes. The two focused kqueue tests pass, and mypy is clean for linux, darwin, and win32.
|
Thanks for all the work! I'm going to close this in favor of #3519 since it solves this and more. |
Fixes #3500.
On kqueue backends, guest mode can fetch an I/O event and then run a host callback before Trio processes that event. If the callback cancels the corresponding wait, the successful abort removes its registration, leaving the already-fetched event with no matching key when
process_events()runs.Track keys removed by successful cancellation.
process_events()snapshots and clears that set for the current batch, and ignores a missing registration only when its key was cancelled in that window. Unrelated unknown keys still raiseKeyErrorrather than being silently swallowed.The regression test reproduces the guest-mode cancellation race on kqueue, with a separate test pinning the unknown-key behavior. CI is green and Codecov reports full coverage of the changed lines.