New signal API: trio.open_signal_receiver #619
Conversation
Fixes python-triogh-354 Other changes: - deprecate trio.catch_signal - fix a few small edge-cases I noticed along the way
Codecov Report
@@ Coverage Diff @@
## master #619 +/- ##
==========================================
+ Coverage 99.31% 99.32% +<.01%
==========================================
Files 91 91
Lines 10873 10933 +60
Branches 758 763 +5
==========================================
+ Hits 10799 10859 +60
Misses 56 56
Partials 18 18
Continue to review full report at Codecov.
|
| @@ -1,11 +1,15 @@ | |||
| import signal | |||
| from contextlib import contextmanager | |||
| from collections import OrderedDict | |||
There was a problem hiding this comment.
I guess whenever 3.5 support is dropped (as soon as pypy supports 3.6?) you can just use a plain ol' dict since 3.7 ruled it stays.
There was a problem hiding this comment.
PyPy has actually had ordered dicts by default for a while (CPython copied their implementation). But annoyingly, we also need popitem(last=False), which even in 3.7 is still an OrderedDict-only method.
There was a problem hiding this comment.
which even in 3.7 is still an OrderedDict-only method.
Ahh what!?
|
|
||
|
|
||
| async def test_catch_signals(): | ||
| # Delete when catch_signals is removed |
There was a problem hiding this comment.
TODO might be handy for grep-ing?
There was a problem hiding this comment.
Eh, when we remove catch_signals the test will start failing, so we won't miss it :-)
| await _core.wait_all_tasks_blocked() | ||
| signal_raise(signal.SIGILL) | ||
| await _core.wait_all_tasks_blocked() | ||
| async for signum in receiver: # pragma: no branch |
There was a problem hiding this comment.
Is it worth ensuring that receiver only once delivers a signal raised multiple times prior to iteration?
I guess I'm just thinking down the road if the implementation is changed (internal set-like data structure or whatever) and this behaviour should remain.
There was a problem hiding this comment.
We are implicitly checking that signals are coalesced, because if they weren't then when we exited the with block they'd be redelivered and cause the test process to dump core :-).
It would make sense to test this a little more explicitly... I'm going to add a little private helper to do that.
There was a problem hiding this comment.
Ahh right of course. Cool, I like explicit 👍
| token = _core.current_trio_token() | ||
| token.run_sync_soon(ev.set, idempotent=True) | ||
| await ev.wait() | ||
|
|
||
| print(1) |
There was a problem hiding this comment.
Maybe print something more descriptive?
There was a problem hiding this comment.
Hmm, actually, have you considered just adding some branch logic for each
await wait_run_sync_soon_idempotent_queue_barrier() location and then just use pytest.mark.parametrize() over signals and that?
Might reduce some code unless the duplication is intentional.
There was a problem hiding this comment.
This is a pretty complicated test that I didn't really touch here, so honestly I'm not sure what it does right now :-). I doubt it's worth messing with – if it breaks we can easily add better print statements or whatever then.
- Add a private test helper to check how many signals are pending - Use this to explicitly test that coalescing has worked - Use this instead of poking at the object's guts directly in existing tests
|
Added a more explicit test of signal coalescing. All comments addressed. |
newsfragments/354.removal.rst
Outdated
| @@ -0,0 +1,8 @@ | |||
| ``trio.signal_catcher`` has been deprecated in favor of | |||
There was a problem hiding this comment.
Looking at the rest of this PR, it looks like the function was called trio.catch_signals?
njsmith
force-pushed
the
open-signal-receiver
branch
from
b87729e
to
fac80e1
Compare
|
Merging since @tgoodlet approved this |
Fixes gh-354
Other changes: