Skip to content
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.

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

gen.with_timeout: Don't log CancelledError after timeout #2681

Merged
merged 2 commits into from
Jun 22, 2019

Conversation

encukou
Copy link
Contributor

@encukou encukou commented Jun 19, 2019

Hello!
I'm not very familiar with Tornado, but I'm investigating the failure on Python 3.8 (#2677).
From commit a237a99, it seems that CancelledError should also be ignored in tornado.gen.with_timeout's error_callback. Is that right?

This fixed tests on Python 3.8.0b1 for me.

bdarnell added a commit to bdarnell/tornado that referenced this pull request Jun 22, 2019
python/cpython#13528 broke us in two ways: asyncio.CancelledError is
no longer an alias for concurrent.futures.CancelledError and it's now
a BaseException.

Fixes tornadoweb#2677
Closes tornadoweb#2681
@bdarnell
Copy link
Member

I'm not sure whether we want to automatically "quiet" CancelledErrors or not. We explicitly pass CancelledError as a quiet_exception here:

timeout, fut, quiet_exceptions=(CancelledError,)

I think as an immediate fix it's better to do something like #2683 to restore the old behavior. Maybe in the longer term we want something like this to always silence CancelledError.

@bdarnell
Copy link
Member

On second thought (see discussion in #2683), I think this is what we want here. I'll merge this PR and then use that one to do a little more cleanup (removing the now-useless references to CancelledError in locks.py, re-enabling 3.8 CI)

@bdarnell bdarnell merged commit dbc00f9 into tornadoweb:master Jun 22, 2019
@encukou encukou deleted the gen-ignore-cancel branch June 24, 2019 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants