Fix error when a shared event loop is unset #1180
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1180 +/- ##
==========================================
- Coverage 88.88% 87.68% -1.21%
==========================================
Files 2 2
Lines 405 414 +9
Branches 44 45 +1
==========================================
+ Hits 360 363 +3
- Misses 35 41 +6
Partials 10 10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
tjkuson
force-pushed
the
mainthread-fix
branch
2 times, most recently
from
7fa2f06 to
5ff08a0
Compare
| _loop = _get_event_loop_no_warn() | ||
| except RuntimeError: | ||
| # Handle situation where asyncio.set_event_loop(None) removes shared loops. | ||
| _reinstate_event_loop_on_main_thread() |
There was a problem hiding this comment.
The function _get_event_loop_no_warn is a helper that suppresses a call to get_event_loop, which is effectively deprecated in favor of get_running_loop. This deprecation was one of the drivers for removing the event_loop fixture, which relied on this functionality.
The fact that wrap_in_sync still uses this is unfortunate and I hope to rectify this in #1203.
The point I'm trying to make is that wrap_in_sync seems to be the wrong place to reinstate the loop, because it would mean doubling down on the get_event_loop use.
| @@ -602,6 +603,12 @@ def _set_event_loop(loop: AbstractEventLoop | None) -> None: | |||
| asyncio.set_event_loop(loop) | |||
|
|
|||
|
|
|||
| def _reinstate_event_loop_on_main_thread() -> None: | |||
| if threading.current_thread() is threading.main_thread(): | |||
There was a problem hiding this comment.
My understanding is that pytest doesn't support multi-threading. It doesn't hurt to have this check, but I wonder if you had a specific use case in mind that I'm missing. Is there? :)
Automatically reinstates the event loop on the main thread when needed.
We reinstate in
wrap_in_sync()as an event loop is required here; without reinstating the event loop here, the next async test will fail to run.The coverage report suggests the patch is uncovered, but I think this is due to limitations of measuring coverage across processes.
Fixes #1177
Note: this is a work-in-progress proposal based on my understanding of the issue. If the maintainers prefer a different approach or consider the issue invalid, I’m happy to adjust or close this PR.