-
-
Notifications
You must be signed in to change notification settings - Fork 338
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Took a little while to get through but I think this looks darn good!
I a have couple small comments that definitely aren't blocking.
@@ -1,11 +1,15 @@ | |||
import signal | |||
from contextlib import contextmanager | |||
from collections import OrderedDict |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PyPy has actually had ordered dict
s 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO
might be handy for grep
-ing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe print something more descriptive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the rest of this PR, it looks like the function was called trio.catch_signals?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😧
b87729e
to
fac80e1
Compare
Merging since @tgoodlet approved this |
Fixes gh-354
Other changes: