Skip to content

Conversation

@vstinner
Copy link
Member

@vstinner vstinner commented Nov 3, 2023

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

Copy link
Contributor

@willingc willingc left a comment

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

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
@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
…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 only security fixes label Nov 4, 2023
vstinner added a commit to miss-islington/cpython that referenced this pull request Nov 4, 2023
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
…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
…#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
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
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

Labels

skip news tests Tests in the Lib/test dir topic-asyncio

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants