gh-121468: Ensure PDB cleans up event loop policies after using asyncio. - #131388
gh-121468: Ensure PDB cleans up event loop policies after using asyncio.#131388freakboy3742 merged 3 commits into
Conversation
Sorry, something went wrong.
|
!buildbot iOS|Android |
Sorry, something went wrong.
|
🤖 New build scheduled with the buildbot fleet by @freakboy3742 for commit e8ed3a8 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F131388%2Fmerge The command will test the builders whose names match following regular expression: iOS|Android The builders matched are:
|
Sorry, something went wrong.
|
Hmm, I have some doubts to do this in PdbTestInput. For the majority of tests, this is not needed. Well actually there's only one test that needs it. It might be benign but it just feels weird to me to do it in such a common function. Do you mind just putting it in the actual test? |
Sorry, something went wrong.
True - but AFAICT, it should be a benign (and fast) cleanup; and it guarantees that any PDB test that triggers asyncio activity will guarantee that it will be cleaned up automatically, rather than requiring test developers to remember that asyncio cleanup is needed on a per-test basis. This is especially significant for iOS and Android, because they often end up as the "canary" identifying issues with sequential test execution. There's nothing iOS- or Android-speciifc about this test or test failure, but the iOS and Android buildbots are the only ones that reveal the problem because they're the only test configurations that actually run the tests sequentially. Anything we can do to systematically prevent this class of failure in the future is a win for me because I don't have to chase down buildbot failures that aren't actually caused by iOS- or Android-specific issues. |
Sorry, something went wrong.
|
The reason it feels weird to me is that this piece of code does not belong there. PdbTestInput has a clear semantics and it does not include cleaning up test residues. I think a better place would be tests.addTest(doctest.DocTestSuite(test_pdb, setUp=setUpPdbBackend('monitoring'))) (make sure you updated to the latest main as this was just merged). You can put a tearDown there which set the policy to None - that makes more sense to me. |
Sorry, something went wrong.
Agreed - that makes more sense. I've made that modification. |
Sorry, something went wrong.
|
!buildbot iOS|Android |
Sorry, something went wrong.
|
🤖 New build scheduled with the buildbot fleet by @freakboy3742 for commit 6c794e0 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F131388%2Fmerge The command will test the builders whose names match following regular expression: iOS|Android The builders matched are:
|
Sorry, something went wrong.
|
!buildbot iOS |
Sorry, something went wrong.
|
🤖 New build scheduled with the buildbot fleet by @freakboy3742 for commit 584aa2b 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F131388%2Fmerge The command will test the builders whose names match following regular expression: iOS The builders matched are:
|
Sorry, something went wrong.
|
So we are doing a double insurance here. |
Sorry, something went wrong.
|
@gaogaotiantian Following discussion with @graingert on Discord, I've modified this to use a two-prong approach:
@graingert mentioned he has an in-progress PR to fail if the async.run() API is used in the "non-clean" (i.e., not passing the loop factory) way; until that lands, the 2 prong approach gives some safety that iOS and Android buildbots won't break. |
Sorry, something went wrong.
… asyncio. (python#131388) Adds teardown logic, plus a change to asyncio.run usage, to avoid warnings when running the test suite single process.
#124367 added a PDB test that interacts with asyncio. Under some conditions, this can lead to a warning during test execution because the PDB test "alters the execution environment" by setting an event loop policy:
$ python.exe -m test test_asyncio.test_unix_events test_pdb Using random seed: 451824264 0:00:00 load avg: 15.60 Run 2 tests sequentially in a single process 0:00:00 load avg: 15.60 [1/2] test_asyncio.test_unix_events 0:00:00 load avg: 15.55 [1/2] test_asyncio.test_unix_events passed 0:00:00 load avg: 15.55 [2/2] test_pdb Warning -- asyncio.events._event_loop_policy was modified by test_pdb Warning -- Before: None Warning -- After: <asyncio.unix_events._UnixDefaultEventLoopPolicy object at 0x101d80190> 0:00:08 load avg: 15.51 [2/2/1] test_pdb failed (env changed) == Tests result: SUCCESS == 1 test altered the execution environment (env changed): test_pdb 1 test OK. Total duration: 8.1 sec Total tests: run=282 skipped=2 Total test files: run=2/2 env_changed=1 Result: SUCCESSThis problem only manifests if you run the tests suite sequentially, or if an asyncio test is performed in the same process as the test_pdb test. iOS and Android tests are always run sequentially, so those platforms are hitting this problem reliably.
This fix ensures that test_pdb cleans up the event policy at the end of each test. The approach I've taken seems consistent with other "end of test cleanup" methods in test_asyncio.