Skip to content

bpo-36916: Swallow unhandled exception #13313

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
May 14, 2019
Merged

bpo-36916: Swallow unhandled exception #13313

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7d6adda
Swallow unhandled exception
asvetlov May 14, 2019
3e515f3
Add NEWS
asvetlov May 14, 2019
0be9d43
make patchcheck
asvetlov May 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion Lib/asyncio/streams.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,13 @@ def __del__(self):
closed.exception()


def _swallow_unhandled_exception(task):
# 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()
Copy link
Member

Choose a reason for hiding this comment

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

Silently ignore exceptions looks like a bad idea. If the task failed, the exception should be at least logged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have a special case here.
Basically, there are two ways to call write() now:

  1. Legacy writer.write(b'data')
  2. 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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

I'm trying to do exactly the opposite than your PR :-) Provide a way to get ignored exceptions: #13187 :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.



class StreamWriter:
"""Wraps a Transport.

Expand Down Expand Up @@ -393,7 +400,9 @@ def _fast_drain(self):
# fast path, the stream is not paused
# no need to wait for resume signal
return self._complete_fut
return self._loop.create_task(self.drain())
ret = self._loop.create_task(self.drain())
ret.add_done_callback(_swallow_unhandled_exception)
return ret

def write_eof(self):
return self._transport.write_eof()
Expand Down
3 changes: 3 additions & 0 deletions Lib/test/test_asyncio/test_streams.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -851,6 +851,8 @@ def test_drain_raises(self):
# where it never gives up the event loop but the socket is
# closed on the server side.

messages = []
self.loop.set_exception_handler(lambda loop, ctx: messages.append(ctx))
q = queue.Queue()

def server():
Expand Down Expand Up @@ -883,6 +885,7 @@ async def client(host, port):
# Clean up the thread. (Only on success; on failure, it may
# be stuck in accept().)
thread.join()
self.assertEqual([], messages)

def test___repr__(self):
stream = asyncio.StreamReader(loop=self.loop,
Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Library/2019-05-14-15-39-34.bpo-36916._GPsTt.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Remove a message about an unhandled exception in a task when writer.write()
is used without await and writer.drain() fails with an exception.