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.

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

Jump to bottom

gh-111644: Fix asyncio test_unhandled_exceptions() #111713

Merged
merged 1 commit into from
Nov 4, 2023
Merged

gh-111644: Fix asyncio test_unhandled_exceptions() #111713

merged 1 commit into from
Nov 4, 2023

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Nov 3, 2023 •
edited by bedevere-app bot
Loading

Fix test_unhandled_exceptions() of test_asyncio.test_streams: break explicitly a reference cycle.

@vstinner vstinner added tests Tests in the Lib/test dir topic-asyncio skip news labels Nov 3, 2023
@bedevere-app bedevere-app bot mentioned this pull request Nov 3, 2023
Closed
@vstinner vstinner added the needs backport to 3.12 bug and security fixes label Nov 3, 2023
willingc
willingc approved these changes Nov 3, 2023
View reviewed changes
Copy link
Contributor

@willingc willingc left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM. Thanks @vstinner

Looks like mac tests are unhappy.

@vstinner
Copy link
Member Author

vstinner commented Nov 3, 2023

Without this change, I can reproduce the issue in a few seconds with the command:

./python -m test test_asyncio.test_streams -m test_unhandled_exceptions -v -j10 --fail-env-changed -F

With this change, it's more difficult to reproduce the issue. Sadly, I can still reproduce it :-(

@kumaraditya303: In Python, storing an exception is likely to create reference cycles. Your test stores exceptions in messages.

@vstinner
Copy link
Member Author

vstinner commented Nov 3, 2023

Ah, my fix is really incomplete since macOS CI failed with:

test_unhandled_exceptions (test.test_asyncio.test_streams.StreamTests.test_unhandled_exceptions) ... Warning -- threading_cleanup() failed to cleanup 1 threads (count: 1, dangling: 2)
Warning -- Dangling thread: <_MainThread(MainThread, started 4552574464)>
Warning -- Dangling thread: <Thread(asyncio_0, started 123145383186432)>
ok

@vstinner
pythongh-111644: Fix asyncio test_unhandled_exceptions()
c855b53 
Fix test_unhandled_exceptions() of test_asyncio.test_streams: break
explicitly a reference cycle.

Fix also StreamTests.tearDown(): the loop must not be closed
explicitly, but using set_event_loop() which takes care of shutting
down the executor with executor.shutdown(wait=True).
BaseEventLoop.close() calls executor.shutdown(wait=False).
@vstinner vstinner force-pushed the test_unhandled_exceptions branch from a755131 to c855b53 Compare November 4, 2023 00:22
@vstinner
Copy link
Member Author

vstinner commented Nov 4, 2023

Aha, I think that I found the root issue: when a loop uses the default executor, it must not be called by loop.close() which uses a weak executor.shutdown(wait=False). But it should just rely on set_event_loop() which registers a cleanup function which calls executor.shutdown(wait=True).

Well, I'm not sure that asyncio executor.shutdown(wait=True) at the first place. IMO it's wrong, it should always wait for threads. But that can be solved separately.

@vstinner vstinner enabled auto-merge (squash) November 4, 2023 00:44
@vstinner vstinner merged commit ac01e22 into python:main Nov 4, 2023
20 checks passed
@vstinner vstinner deleted the test_unhandled_exceptions branch November 4, 2023 00:47
@miss-islington-app
Copy link

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 4, 2023
@vstinner @miss-islington
pythongh-111644: Fix asyncio test_unhandled_exceptions() (pythonGH-11…
50596df 
…1713)

Fix test_unhandled_exceptions() of test_asyncio.test_streams: break
explicitly a reference cycle.

Fix also StreamTests.tearDown(): the loop must not be closed
explicitly, but using set_event_loop() which takes care of shutting
down the executor with executor.shutdown(wait=True).
BaseEventLoop.close() calls executor.shutdown(wait=False).
(cherry picked from commit ac01e22)

Co-authored-by: Victor Stinner <vstinner@python.org>
@bedevere-app
Copy link

bedevere-app bot commented Nov 4, 2023

GH-111718 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 bug and security fixes label Nov 4, 2023
@vstinner vstinner mentioned this pull request Nov 4, 2023
Merged
vstinner added a commit to miss-islington/cpython that referenced this pull request Nov 4, 2023
@vstinner
pythongh-111644: Fix asyncio test_unhandled_exceptions() (python#111713)
dced4b8 
Fix test_unhandled_exceptions() of test_asyncio.test_streams: break
explicitly a reference cycle.

Fix also StreamTests.tearDown(): the loop must not be closed
explicitly, but using set_event_loop() which takes care of shutting
down the executor with executor.shutdown(wait=True).
BaseEventLoop.close() calls executor.shutdown(wait=False).

(cherry picked from commit ac01e22)
vstinner added a commit that referenced this pull request Nov 4, 2023
@miss-islington @kumaraditya303 @vstinner
[3.11] [3.12] GH-110894: Call loop exception handler for exceptions i…
f7ffe4a 
…n client_connected_cb (GH-111601) (GH-111632) (#111634)

* [3.12] GH-110894: Call loop exception handler for exceptions in client_connected_cb (GH-111601) (GH-111632)
(cherry picked from commit 9aa8829)

Co-authored-by: Kumar Aditya <kumaraditya@python.org>
Call loop exception handler for exceptions in `client_connected_cb` of `asyncio.start_server` so that applications can handle it..
(cherry picked from commit 229f44d)

* gh-111644: Fix asyncio test_unhandled_exceptions() (#111713)

Fix test_unhandled_exceptions() of test_asyncio.test_streams: break
explicitly a reference cycle.

Fix also StreamTests.tearDown(): the loop must not be closed
explicitly, but using set_event_loop() which takes care of shutting
down the executor with executor.shutdown(wait=True).
BaseEventLoop.close() calls executor.shutdown(wait=False).

(cherry picked from commit ac01e22)

---------

Co-authored-by: Kumar Aditya <kumaraditya@python.org>
Co-authored-by: Victor Stinner <vstinner@python.org>
vstinner added a commit that referenced this pull request Nov 4, 2023
@miss-islington @vstinner
[3.12] gh-111644: Fix asyncio test_unhandled_exceptions() (GH-111713) (…
d15fac2 
…#111718)

gh-111644: Fix asyncio test_unhandled_exceptions() (GH-111713)

Fix test_unhandled_exceptions() of test_asyncio.test_streams: break
explicitly a reference cycle.

Fix also StreamTests.tearDown(): the loop must not be closed
explicitly, but using set_event_loop() which takes care of shutting
down the executor with executor.shutdown(wait=True).
BaseEventLoop.close() calls executor.shutdown(wait=False).
(cherry picked from commit ac01e22)

Co-authored-by: Victor Stinner <vstinner@python.org>
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
@vstinner @aisk
pythongh-111644: Fix asyncio test_unhandled_exceptions() (python#111713)
265e089 
Fix test_unhandled_exceptions() of test_asyncio.test_streams: break
explicitly a reference cycle.

Fix also StreamTests.tearDown(): the loop must not be closed
explicitly, but using set_event_loop() which takes care of shutting
down the executor with executor.shutdown(wait=True).
BaseEventLoop.close() calls executor.shutdown(wait=False).
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
@vstinner @Glyphack
pythongh-111644: Fix asyncio test_unhandled_exceptions() (python#111713)
7e3e96f 
Fix test_unhandled_exceptions() of test_asyncio.test_streams: break
explicitly a reference cycle.

Fix also StreamTests.tearDown(): the loop must not be closed
explicitly, but using set_event_loop() which takes care of shutting
down the executor with executor.shutdown(wait=True).
BaseEventLoop.close() calls executor.shutdown(wait=False).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@willingc willingc willingc approved these changes

@1st1 1st1 Awaiting requested review from 1st1 1st1 is a code owner

@asvetlov asvetlov Awaiting requested review from asvetlov asvetlov is a code owner

@gvanrossum gvanrossum Awaiting requested review from gvanrossum

@kumaraditya303 kumaraditya303 Awaiting requested review from kumaraditya303 kumaraditya303 is a code owner

Assignees
No one assigned
Labels
skip news tests Tests in the Lib/test dir topic-asyncio
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@vstinner @willingc