Skip to content

bpo-39765: Document caveats for asyncio add_signal_handler() - #18664

Closed
rogerdahl wants to merge 5 commits into
python:mainfrom
rogerdahl:master
Closed

rogerdahl wants to merge 5 commits into
python:mainfrom
rogerdahl:master

Conversation

@rogerdahl

@rogerdahl rogerdahl commented Feb 26, 2020 •

Copy link
Copy Markdown

Add information about potential delays in callbacks registered by this function, and a note about the side effect that corresponding handlers registered directly by signal.signal() are implicitly removed.

https://bugs.python.org/issue39765

Add information about potential delays in callbacks registered by this function, and a note about the side effect that corresponding handlers registered directly by signal.signal() are implicitly removed.
Add caveats to add_signal_handler()
@the-knights-who-say-ni

Copy link
Copy Markdown

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

Recognized GitHub username

We couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames:

@rogerdahl

This might be simply due to a missing "GitHub Name" entry in one's b.p.o account settings. This is necessary for legal reasons before we can look at this contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

@bedevere-bot bedevere-bot added docs Documentation in the Doc dir awaiting review labels Feb 26, 2020

@asvetlov asvetlov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Please create an issue on https://bugs.python.org
  2. The text is slightly incorrect.

While signal handlers registered using :func:signal.signal will
typically be called immediately
Actually in Python signal handlers are called with some delay in the main thread context. Only C-level sig handlers are called immediately with limited sighandler stack. C handler switches on a flag and asks the main thread to execute the actual Python-level handler as soon as possible. asyncio is not different but the typical delay is greater, sure.

@bedevere-bot

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@rogerdahl rogerdahl changed the title Add caveats for asyncio add_signal_handler() doc Document caveats for asyncio add_signal_handler() bpo-39765 Feb 26, 2020
@rogerdahl

Copy link
Copy Markdown
Author

Done.

  • The text is slightly incorrect.

What I was trying to get across was that the delays in callbacks registered by signal.signal() typically won't be perceivable. Feel free to improve the wording.

Comment thread Doc/library/asyncio-eventloop.rst Outdated
thread.

While signal handlers registered using :func:`signal.signal` will
typically be called immediately, handlers registered using this

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not true. There are no such guarantees for signal.signal callback. E.g. a long-running C function can block Python signals until it completes its execution.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know, but that's why I used the "typically" qualifier and didn't say that immediate callbacks are guaranteed.

I do feel like it's probably safe to say that the typical user experience with :func:signal.signal is that there's no perceptible delay, since the same delays would affect the default handlers for Ctrl-C / SIGINT and be very noticeable. I don't know if my experience with delays in callbacks registered with :func:add_signal_handler is typical though.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know, but that's why I used the "typically" qualifier and didn't say that immediate callbacks are guaranteed.

"immediate" is a very strong word that "typically" doesn't really soften :) Again, signal callbacks are not called "immediately" by any typical or atypical definition. That's why I don't like this wording. My 2c.

no perceptible delay

There might be no perceptible delay, but the delay can still be fractions of a second which is infinitely long from the CPU perspective.

The real reason of this perceptible delay is that signal callbacks in asyncio are executed as all other callbacks - when they go through the queue. And if the callbacks queue is long it might take them longer to execute than ones registered via signal.signal. I suggest explaining from this perspective.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interestingly, I noticed some stuff currently in CI that might address it for my use case, which is that I want the program to stop right away when I hit Ctrl-C. The change adds special handling for SIGINT in the loop.

How about wording:

While signal handlers registered using :func:signal.signal will typically be called without significant delays as seen from the perspective of an interactive user, signal callbacks in asyncio are subject to additional delays while waiting for other work in the given event loop to be completed.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the new wording!

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noticed that I did the PR from master. Sorry about that.

Comment thread Doc/library/asyncio-eventloop.rst Outdated
@rogerdahl

Copy link
Copy Markdown
Author

I have made the requested changes; please review again.

@bedevere-bot

Copy link
Copy Markdown

Thanks for making the requested changes!

@asvetlov: please review the changes made to this pull request.

@rogerdahl

Copy link
Copy Markdown
Author

@asvetlov Just a reminder to review and hopefully merge this.

@erlend-aasland erlend-aasland changed the title Document caveats for asyncio add_signal_handler() bpo-39765 bpo-39765: Document caveats for asyncio add_signal_handler() May 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting change review docs Documentation in the Doc dir

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants