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

bpo-39101: Fixes BaseException hang in IsolatedAsyncioTestCase. #22654

Merged
merged 3 commits into from
Oct 26, 2020

Conversation

lisroach
Copy link
Contributor

@lisroach lisroach commented Oct 11, 2020

If a BaseException is raised in IsolatedAsyncioTestCase the test will hang forever. This should fix that, added a unit test that would hang forever without the fix.

https://bugs.python.org/issue39101

@@ -104,7 +104,7 @@ async def _asyncioLoopRunner(self, fut):
fut.set_result(ret)
except asyncio.CancelledError:
Copy link
Member

Choose a reason for hiding this comment

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

Please add except (SystemExit, KeyboardInterrupt): raise before handling the BaseException.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@1st1 this will still leave SystemExit and KeyboardInterrupt hanging if they are raised, I think they should be handled along with the BaseException.

Copy link
Member

@1st1 1st1 left a comment

Choose a reason for hiding this comment

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

lgtm but please take a look at the suggested change.

@danielnelson
Copy link

Should the except where we reraise asyncio.CancelledError be removed too? I ran into a hang with a test like:

import asyncio
import unittest

class TestHangsForever(unittest.IsolatedAsyncioTestCase):
    async def test_hangs_forever(self):
        task = asyncio.create_task(asyncio.sleep(1))
        task.cancel()
        await task

if __name__ == "__main__":
    import unittest
    unittest.main()

@lisroach
Copy link
Contributor Author

@danielnelson you are right, moving asyncio.CancelledError to be handled the same way as BaseException fixes the hang.

@1st1 or @asvetlov, I am confused why KeyboardInterrupt and SystemExit are not handled along with BaseException. I read a little of python/asyncio#341 that seems to indicate there are some fundamental differences in how they are handled. I also see issue40152 that seems to reference the hanging problem.
A test like:

async def test_sys_exit(self):
    raise SystemExit()

Just hangs forever. Should I file a new bug for this?

@1st1
Copy link
Member

1st1 commented Oct 25, 2020

@1st1 or @asvetlov, I am confused why KeyboardInterrupt and SystemExit are not handled along with BaseException. I read a little of python/asyncio#341 that seems to indicate there are some fundamental differences in how they are handled. I also see issue40152 that seems to reference the hanging problem.

They are specially handled because asyncio has no way of correctly recovering from them. They can happen at any time (including when asyncio runs its own private implementation code) wrecking the internal state. So if asyncio sees them it just propagates them immediately with the intent to crash early.

This is something we'd like to fix eventually, but it will require some substantial internal changes. For now I suggest to treat them just like they're treated in asyncio (propagate them immediately).

Copy link
Member

@1st1 1st1 left a comment

Choose a reason for hiding this comment

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

The fix looks correct to me.

Copy link
Contributor

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

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

LGTM!

@asvetlov asvetlov added the needs backport to 3.9 only security fixes label Oct 25, 2020
@danielnelson
Copy link

In addition to the bug listed in the original comment, I believe this PR also fixes https://bugs.python.org/issue39706

@lisroach lisroach merged commit 8374d2e into python:master Oct 26, 2020
@miss-islington
Copy link
Contributor

Thanks @lisroach for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8, 3.9.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-22988 is a backport of this pull request to the 3.9 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 26, 2020
…onGH-22654)

(cherry picked from commit 8374d2e)

Co-authored-by: Lisa Roach <lisaroach14@gmail.com>
@bedevere-bot
Copy link

GH-22989 is a backport of this pull request to the 3.8 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 26, 2020
…onGH-22654)

(cherry picked from commit 8374d2e)

Co-authored-by: Lisa Roach <lisaroach14@gmail.com>
shihai1991 added a commit to shihai1991/cpython that referenced this pull request Oct 29, 2020
…lots1

* origin/master: (365 commits)
  bpo-42029: Remove IRIX code (pythonGH-23023)
  bpo-42143: Ensure PyFunction_NewWithQualName() can't fail after creating the func object (pythonGH-22953)
  bpo-34204: Use pickle.DEFAULT_PROTOCOL in shelve (pythonGH-19639)
  bpo-41805: Documentation for PEP 585 (pythonGH-22615)
  bpo-42161: Micro-optimize _collections._count_elements() (pythonGH-23008)
  bpo-42161: Remove private _PyLong_Zero and _PyLong_One (pythonGH-23003)
  bpo-42099: Fix reference to ob_type in unionobject.c and ceval (pythonGH-22829)
  bpo-41659: Disallow curly brace directly after primary (pythonGH-22996)
  bpo-6761: Enhance __call__ documentation (pythonGH-7987)
  bpo-42161: Modules/ uses _PyLong_GetZero() and _PyLong_GetOne() (pythonGH-22998)
  bpo-41474, Makefile: Add dependency on cpython/frameobject.h (pythonGH-22999)
  bpo-42157: Rename unicodedata.ucnhash_CAPI (pythonGH-22994)
  bpo-42161: Use _PyLong_GetZero() and _PyLong_GetOne() (pythonGH-22995)
  bpo-30681: Support invalid date format or value in email Date header (pythonGH-22090)
  bpo-42161: Add _PyLong_GetZero() and _PyLong_GetOne() (pythonGH-22993)
  bpo-42123: Run the parser two times and only enable invalid rules on the second run (pythonGH-22111)
  bpo-42157: Convert unicodedata.UCD to heap type (pythonGH-22991)
  bpo-42157: unicodedata avoids references to UCD_Type (pythonGH-22990)
  bpo-39101: Fixes BaseException hang in IsolatedAsyncioTestCase. (pythonGH-22654)
  bpo-1635741: _PyUnicode_Name_CAPI moves to internal C API (pythonGH-22713)
  ...
miss-islington added a commit that referenced this pull request Dec 16, 2020
…2654)

(cherry picked from commit 8374d2e)

Co-authored-by: Lisa Roach <lisaroach14@gmail.com>
miss-islington added a commit that referenced this pull request Dec 16, 2020
…2654)

(cherry picked from commit 8374d2e)

Co-authored-by: Lisa Roach <lisaroach14@gmail.com>
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
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.

7 participants