asyncio overrides signal handlers

[df2c16d9-e835-4bf0-a373-441d4d104450]

BPO	44212
Nosy	@asvetlov, @1st1, @franciscod
PRs	python/cpython#26306

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields: ```python assignee = None closed_at = None created_at = labels = ['type-bug', '3.8', '3.9', '3.10', '3.11', '3.7', 'expert-asyncio'] title = 'asyncio overrides signal handlers' updated_at = user = 'https://github.com/franciscod' ``` bugs.python.org fields: ```python activity = actor = 'Francisco Demartino' assignee = 'none' closed = False closed_date = None closer = None components = ['asyncio'] creation = creator = 'Francisco Demartino' dependencies = [] files = [] hgrepos = [] issue_num = 44212 keywords = ['patch'] message_count = 2.0 messages = ['394174', '394176'] nosy_count = 3.0 nosy_names = ['asvetlov', 'yselivanov', 'Francisco Demartino'] pr_nums = ['26306'] priority = 'normal' resolution = None stage = 'patch review' status = 'open' superseder = None type = 'behavior' url = 'https://bugs.python.org/issue44212' versions = ['Python 3.6', 'Python 3.7', 'Python 3.8', 'Python 3.9', 'Python 3.10', 'Python 3.11'] ```

[df2c16d9-e835-4bf0-a373-441d4d104450]

Hello,

It looks like when asyncio sets up a signal handler, it forgets about the previous one (if any).

Here's a patch (was about to create a PR but the default text brought me to bugs.python.org) https://github.com/franciscod/cpython/commit/bdac885b86fbb01d0d775f40c47870e48af5fa5b

Tracked this down to the initial asyncio checkout, in commit 27b7c7ebf1039e96cac41b6330cf16b5632d9e49, a few commits before v3.4.0a4.

Not sure if this is a bug but it surprised me. I would have expected that it registered an "additional" signal handler, or at least that my previously installed signal handler would be called afterwards.

Also I'm not sure that it's a good idea to suddenly start running handlers that some current code might rely on not running, or have worked around this behaviour otherwise.

Thanks, Francisco

[df2c16d9-e835-4bf0-a373-441d4d104450]

Looks like Roger Dahl also noted this on https://bugs.python.org/issue39765:

Second, set_signal_handler()[sic] silently and implicitly removes corresponding handlers set with signal.signal(). [...] I think this should be documented as well.

(then has a linked PR that apparently doesn't address this second part)

[gvanrossum]

FWIW, last week @1st1 and I discussed a new signal handling API for asyncio, where you would use register_signal_handler(sig, callback) which would return an asyncio.Handle and would allow any number of handlers to be set for the same signal. To unregister, just cancel the handle.

However that still doesn't address the fact that asyncio just clobbers an existing handler set by the signal module. If we were to add a similar API to that module (using some other kind of handle-like object) we could switch asyncio to use that.

FWIW you can go down this rabbit hole ever deeper. E.g. I also found https://github.com/python/cpython/issues/76624, suggesting to add Linux's signalfd.

[kumaraditya303]

@gvanrossum Want to work on this? I am not sure that we need another set of APIs for this though. I can fix this without introducing new APIs and that could also be backported.

[gvanrossum]

I don't have time to work on it. I agree that more APIs is not always better, but I'm not sure how you'd change this behavior and call it backwards compatible, let alone backportable.

What semantics are you proposing for loop.add_signal_handler() and loop.remove_signal_handler()?

[kumaraditya303]

What semantics are you proposing for loop.add_signal_handler() and loop.remove_signal_handler()?

The only part which would be backported is that signal handlers registered with signal.signal will be respected and will be called. We can support multiple signal handlers for 3.12+ if wanted and this part will not be backported.

[gvanrossum]

I'm still not sure about the API you proposed. Suppose I have

signal.signal(SIGUSR1, some_handler)
loop.add_signal_handle(SIGUSR1, another_handler)

Are you saying that both some_handler and another_handler will be called when SIGUSR1 arrives? That sure sounds backwards incompatible, since the current behavior only calls the latter IIRC.

[kumaraditya303]

Are you saying that both some_handler and another_handler will be called when SIGUSR1 arrives? That sure sounds backwards incompatible, since the current behavior only calls the latter IIRC.

Yes, both handlers would be called. asyncio should not override and remove the handler registered via signal.signal.

That sure sounds backwards incompatible, since the current behavior only calls the latter IIRC.

That's arguably the bug to be fixed here. We can ofcourse consider this to be a new feature and chose to not backport this.

P.S In the second handler it should be SIGUSR1 not SIGUSR2.

[gvanrossum]

Are you saying that both some_handler and another_handler will be called when SIGUSR1 arrives? That sure sounds backwards incompatible, since the current behavior only calls the latter IIRC.

Yes, both handlers would be called. asyncio should not override and remove the handler registered via signal.signal.

It's been like this since asyncio first got signal handling (3.4?), and surely this behavior has been accepted by user code as "that's how it works" (even if it was never documented).

That sure sounds backwards incompatible, since the current behavior only calls the latter IIRC.

That's arguably the bug to be fixed here. We can ofcourse consider this to be a new feature and chose to not backport this.

I don't think we can change this even in 3.12, it would break code that sets a handler using signal.signal() and then expects to replace that using loop.add_signal_handler().

That's why Yury and I were debating a new API.

P.S In the second handler it should be SIGUSR1 not SIGUSR2.

Thanks, fixed.

[kumaraditya303]

Yeah, adding new APIs looks like the best way forward here.

[willingc]

Next action: Let's discuss new APIs at the core dev sprint.