bpo-36916: Swallow unhandled exception #13313
Conversation
| # Do a trick to suppress unhandled exception | ||
| # if stream.write() was used without await and | ||
| # stream.drain() was paused and resumed with an exception | ||
| task.exception() |
There was a problem hiding this comment.
Silently ignore exceptions looks like a bad idea. If the task failed, the exception should be at least logged.
There was a problem hiding this comment.
We have a special case here.
Basically, there are two ways to call write() now:
- Legacy
writer.write(b'data') - Recommended
await writer.write(b'data')
For the second case, an exception raised by nested writer.drain() call is propagated to user.
The first case ignores the exception from nested writer.drain() call, allowing existing code to work in a backward compatible way.
A subsequent call to drain() raises the exception again, like in the following snippet:
writer.write(b'data') # exception is swallowed
await writer.drain() # previous exception is raised if any
Sorry for such a tricky code.
Ideally, we should make writer.write() a coroutine from the very beginning.
Proposed workaround is a strategy to keep backward compatibility and provide a smooth transition path.
There was a problem hiding this comment.
writer.write(b'data') # exception is swallowed
Again, this exception should be logged somehow. It's a bad practice to silently ignore exceptions and don't provide any way to the developer to see these exceptions.
Can you try to write the exception?
I only merged your PR only to attempt to fix https://bugs.python.org/issue36870
There was a problem hiding this comment.
I'm trying to do exactly the opposite than your PR :-) Provide a way to get ignored exceptions: #13187 :-)
There was a problem hiding this comment.
Well, it is not for suppressing an exception entirely, the exception is raised to the caller on await writer.write() if this form of call is used.
It is for disabling the exception logging. We do similar things in asyncio somewhere.
|
When you're done making the requested changes, leave the comment: |
|
I have made the requested changes; please review again. Well, actually I did nothing but provide a comment that describes why ignoring the exception in this particular case is ok. |
|
Thanks for making the requested changes! @vstinner: please review the changes made to this pull request. |
|
Honestly, I don't understand how it can be a good idea to silently ignore errors. But this PR fix the warning on my Windows VM when running test_drain_raises() on Windows, so I approved it. My main concern right now is that https://bugs.python.org/issue36870 is breaking the Python CI and the Python workflow. |
|
Thanks, @vstinner |
|
@vstinner Please don't merge asyncio PRs without my review. This one looks fine though. |
test_asyncio was causing a lot of troubles, please see https://bugs.python.org/issue36870 for the context. @asvetlov: Can you please also fix Python 3.7? Python 3.7 is also affected by https://bugs.python.org/issue36870 |
|
By the way, it's not the first time that the Python CI and the Python workflow is broken by test_asyncio, especially by random failures on Windows. I'm not sure how to prevent these issues. The policy for the CI is to revert a change which introduces a regression: https://pythondev.readthedocs.io/ci.html#revert-on-fail That's what I did: PR #13316. But @asvetlov asked me to look at this PR instead. |
|
Sorry, @asvetlov and @vstinner, I could not cleanly backport this to |
The PR fixes a message about an unhandled exception in a task when
writer.write()is used without await butwriter.drain()fails with an exception.https://bugs.python.org/issue36916