Skip to content
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

Remove batching from catch_signals? #354

njsmith opened this issue Nov 29, 2017 · 1 comment · Fixed by #619

Remove batching from catch_signals? #354

njsmith opened this issue Nov 29, 2017 · 1 comment · Fixed by #619


Copy link

njsmith commented Nov 29, 2017

I was just looking at catch_signals, and it occurred to me maybe we can improve on its funky batching API.

The original motivation was the same as for UnboundedQueue: since we have no way to put backpressure onto signals as they arrive, we nudge users to handle a whole batch at once.

One alternative I considered were to use a regular UnboundedQueue, which has the problem that its memory use actually be unbounded, which feels wrong given that signals are coalescable and thus actually can be queued with bounded memory. The other was to store incoming signals in a set, and then pop an element each time. The problem with this is that it can lead to a kind of starvation where signals of type A keep coming in and getting popped, while a signal of type B sits in the set indefinitely without being reported. (For example, a policy that would guarantee this worst-case behavior would be to always report the lowest-number pending signal.)

What I realized now though is that there's another option: we could use the set-type approach, but with a policy that's guaranteed to cycle through all the different signals in bounded time. For example, if on each iteration we report the signal with the lowest number that is strictly larger than the one we reported last time (wrapping around as necessary), then we'd guarantee that any incoming signal was reported within 64 event loop iterations at worst (at least on Linux which has 64 signals, but other OSes are similar). (And of course in practice it would be much, much quicker, since you never receive all 64 signals at once.)

Probably the nicest way to accomplish this would be to use an OrderedDict for queueing incoming signals, like we do for TrioToken.run_sync_soon(..., idempotent=True) calls.

There's some transition issue here, since this change would be backwards incompatible in a way that we can't warn for. We can either come up with a new name for catch_signals and transition to using that instead, or else rip off the bandaid by just changing it.

Copy link
Member Author

njsmith commented Dec 1, 2017

Regarding the transition: Maybe we could take this as an opportunity to standardize this on an open_* name, like we use for everything else.

njsmith added a commit to njsmith/trio that referenced this issue Aug 22, 2018
Fixes python-triogh-354

Other changes:

- deprecate trio.catch_signal
- fix a few small edge-cases I noticed along the way
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet

Successfully merging a pull request may close this issue.

1 participant