fix: Gracefully handle event loops in fresh process - #811
Conversation
| try: | ||
| self._event_loop = asyncio.get_running_loop() | ||
| except RuntimeError: | ||
| self._event_loop = asyncio.new_event_loop() |
There was a problem hiding this comment.
This seems kind of awful. Can we do something consistent here instead? What do the tests actually require?
There was a problem hiding this comment.
I agree it's awful.
But it's better than if asyncio.get_event_loop_policy()._local._loop is not None
They don't have a way of checking if there is a current event loop and actually recommend this exact method 🫠
10c3b47 to
4bc73ad
Compare
In Python 3.12/3.13 they deprecated get_event_loop() returning a new loop when none is current. In Python 3.14 it now raises a RuntimeError. Reference: https://docs.python.org/3.14/library/asyncio-eventloop.html#asyncio.get_event_loop
254ec40 to
d994670
Compare
| except RuntimeError as exc: | ||
| raise ScriptWorkerException( | ||
| "No event loop is running and none was set on the context. Set ``context.event_loop`` before use, or read it from a coroutine." | ||
| ) from exc |
There was a problem hiding this comment.
Only change here now is that we raise a ScriptWorkerException instead of letting asyncio raise a RuntimeError
bhearsum
left a comment
bhearsum
left a comment
There was a problem hiding this comment.
I'm r+'ing, but please seriously consider scrapping the noted test, and maybe even dropping the event loop closing to avoid unnecessary complexity (and tests).
| if event_loop is None: | ||
| owned_event_loop = asyncio.new_event_loop() | ||
| context.event_loop = owned_event_loop | ||
| else: |
There was a problem hiding this comment.
It's a tad unfortunate that the only use for this path is in tests...but that's not easy to change considering some of the tests appear to need to re-use the same event loop for other things. Definitely out of scope here.
| finally: | ||
| if owned_event_loop is not None: | ||
| # closing the loop also deregisters the signal handlers added above | ||
| owned_event_loop.close() |
There was a problem hiding this comment.
To be honest, I don't really think there's a need to explicitly close this, which would avoid the need to keep track of it like this. There's no python code that runs after this point...so there's no harm in not explicitly closing it IMO.
Not going to block on this though.
|
|
||
|
|
||
| def test_main_repeated_calls_do_not_leak_loops(mocker, context): | ||
| """Restarting ``main()`` in one process doesn't accumulate open loops.""" |
There was a problem hiding this comment.
note: the only way for main to be called outside of tests is by running scriptworker. There is no way for it to be called more than once per process. IMO you should scrap this test, as it doesn't cover any remotely real scenario.
3 participants
In Python 3.12/3.13 they deprecated get_event_loop() returning a new loop when none is current. In Python 3.14 it now raises a RuntimeError.
Reference: https://docs.python.org/3.14/library/asyncio-eventloop.html#asyncio.get_event_loop