Skip to content

bpo-35749: Don't log exception if event loop wakeup pipe is full#11577

Closed
asvetlov wants to merge 3 commits into
python:masterfrom
asvetlov:dont-log-self-pipe-overload
Closed

bpo-35749: Don't log exception if event loop wakeup pipe is full#11577
asvetlov wants to merge 3 commits into
python:masterfrom
asvetlov:dont-log-self-pipe-overload

Conversation

@asvetlov

@asvetlov asvetlov commented Jan 16, 2019

Copy link
Copy Markdown
Contributor

wakeup = False
while True:
try:
data = self._ssock.recv(4096)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Can use sock.recv_into() here. Not sure if it worth to change

@asvetlov

Copy link
Copy Markdown
Contributor Author

@vstinner the PR is what I meant when commented your PR for logging pipe overflow #11566

@vstinner vstinner left a comment

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.

" bpo-35749: Don't log exception if event loop wakeup pipe is full #11577" title is inaccurate. Your change just completely rewrite how asyncio handles signals.

Ignoring logs when the pipe is full is a weak rationale to rewrite signal handling. Please dig into the history to see why asyncio is written like that. I recall vaguely that there were complex race conditions on FreeBSD.

I'm not against "experimenting" new way to handle signals, but it would need a better rationale.

cc @njsmith @1st1

@bedevere-bot

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@asvetlov

Copy link
Copy Markdown
Contributor Author

warn_on_full_buffer argument was added in Python 3.7
Without it the PR is impossible.

I'll change the title and changelog text if we decide to land the change.
Sure, the PR needs a careful review, any feedback is very appreciated.

Let' me explain the motivation better:

  1. Now we collapse all signals catched on single event loop iteration to handle every signal handler only once. I think it is good, Python itself handles signals asynchronously. There is no reason to call asyncio handler for particular subscribed signal twice on single iteration.
  2. Storing raised signal numbers in a set allows to ignore errors if self pipe is overloaded. We need only one pipe read event to wake up and handle both pending async signals and cross-threads calls (loop.call_soon_threadsafe()).
  3. Failures on writing into self pipe are signs for either high load or blocking/long-running code in callback handlers.
    The former is pretty normal, additional logging doesn't help too much; especially in debug mode (existing checks are skipped in release mode for sake of speed anyway). Most likely, users don't use debug mode for running production code. Moreover, logging the fact that the loop is under high load on every iteration makes the load even worse :)
    Blocking calls are reported by loop._run_once() in debug mode anyway; there is no need to double the message.

@asvetlov asvetlov closed this Jan 16, 2019
@asvetlov
asvetlov deleted the dont-log-self-pipe-overload branch January 16, 2019 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants