GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-36916: Swallow unhandled exception by asvetlov · Pull Request #13313 · python/cpython

Conversation

@asvetlov

@asvetlov asvetlov commented May 14, 2019 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

The PR fixes a message about an unhandled exception in a task when writer.write() is used without await but writer.drain() fails with an exception.

https://bugs.python.org/issue36916

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@asvetlov
Swallow unhandled exception
7d6adda
@asvetlov
asvetlov requested a review from 1st1 as a code owner May 14, 2019 12:37
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label May 14, 2019
@bedevere-bot bedevere-bot added the awaiting core review label May 14, 2019
@asvetlov asvetlov added the topic-asyncio label May 14, 2019
asvetlov added 2 commits May 14, 2019 15:39
@asvetlov
Add NEWS
3e515f3
@asvetlov
make patchcheck
0be9d43
@asvetlov asvetlov mentioned this pull request May 14, 2019
Revert "bpo-36802: Drop awrite()/aclose(), support await write() and await close() instead (#13099)" #13316
Closed
vstinner
vstinner requested changes May 14, 2019
View reviewed changes
Comment thread Lib/asyncio/streams.py
# 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()

@vstinner vstinner May 14, 2019

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@asvetlov asvetlov May 14, 2019

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vstinner vstinner May 14, 2019

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vstinner vstinner May 14, 2019

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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 :-)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@asvetlov asvetlov May 14, 2019

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting changes and removed awaiting core review labels May 14, 2019
@bedevere-bot

bedevere-bot commented May 14, 2019

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@asvetlov

asvetlov commented May 14, 2019

Copy link
Copy Markdown
Contributor Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting change review and removed awaiting changes labels May 14, 2019
@bedevere-bot

bedevere-bot commented May 14, 2019

Copy link
Copy Markdown

Thanks for making the requested changes!

@vstinner: please review the changes made to this pull request.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vstinner
vstinner approved these changes May 14, 2019
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting change review labels May 14, 2019
@vstinner

vstinner commented May 14, 2019

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner
vstinner merged commit f12ba7c into python:master May 14, 2019
@bedevere-bot bedevere-bot removed the awaiting merge label May 14, 2019
@vstinner vstinner mentioned this pull request May 14, 2019
bpo-36763: Add test for _PyCoreConfig_SetString() #13275
Merged
@asvetlov
asvetlov deleted the swallow-unhandled-exception branch May 14, 2019 16:12
@asvetlov

asvetlov commented May 14, 2019

Copy link
Copy Markdown
Contributor Author

Thanks, @vstinner
Regarding random fails: looks like the current asyncio code showed the problem that existed for a long time already.
Please let me warm up my Windows VM, I'll cope with it.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@1st1

1st1 commented May 14, 2019

Copy link
Copy Markdown
Member

@vstinner Please don't merge asyncio PRs without my review. This one looks fine though.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner

vstinner commented May 14, 2019

Copy link
Copy Markdown
Member

@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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner

vstinner commented May 14, 2019

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner vstinner added the needs backport to 3.7 label May 14, 2019
@miss-islington

miss-islington commented May 14, 2019

Copy link
Copy Markdown
Contributor

Thanks @asvetlov for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington

miss-islington commented May 14, 2019

Copy link
Copy Markdown
Contributor

Sorry, @asvetlov and @vstinner, I could not cleanly backport this to 3.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker f12ba7cd0a7370631214ac0b337ab5455ce717b2 3.7
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington miss-islington assigned vstinner May 14, 2019
This was referenced May 14, 2019
[3.7] Revert "bpo-36801: Fix waiting in StreamWriter.drain for closing SSL transport (GH-13098)" #13328
Merged
Revert "bpo-36801: Fix waiting in StreamWriter.drain for closing SSL transport (GH-13098)" #13329
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner approved these changes
@1st1 1st1 Awaiting requested review from 1st1

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@vstinner vstinner

Labels

topic-asyncio

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@asvetlov @bedevere-bot @vstinner @1st1 @miss-islington @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.