fix: replace deprecated asyncio event loop patterns#877
Conversation
|
Warning Review limit reached
Next review available in: 36 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe changes modernize asyncio usage across the Mitmproxy, shell, and TFTP drivers. TFTP server lifecycle management now uses ChangesAsyncio driver updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py`:
- Around line 77-94: Handle setup failures in _run_server_lifecycle by capturing
exceptions from asyncio.get_running_loop() and TftpServer construction in a
startup-error field, then always set _loop_ready so waiting callers are
unblocked. Update start() to check this field after _loop_ready.wait() and raise
the original exception instead of reporting the generic timeout; preserve
cleanup of _loop in the lifecycle finally block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c59dc39b-ba1e-46de-a0b1-7bb3b1ff868d
📒 Files selected for processing (3)
python/packages/jumpstarter-driver-mitmproxy/examples/addons/data_stream_websocket.pypython/packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver.pypython/packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@python/packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver_test.py`:
- Line 1: Remove the unused asyncio import from driver_test.py; no other changes
are needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: df5109f8-b7a7-4540-883c-2b0225cac98d
📒 Files selected for processing (1)
python/packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver_test.py
| This ensures asyncio.Event() objects inside TftpServer are created | ||
| with a running event loop, as required by Python 3.14+. |
There was a problem hiding this comment.
Dumb question maybe, but is this true? The docs say the loop parameter was removed from asyncio.Event in 3.10 and I thought the loop gets picked up lazily on first await since then, so creating it without a running loop should be OK even on 3.14? https://docs.python.org/3/library/asyncio-sync.html#asyncio.Event
There was a problem hiding this comment.
Thanks for the review, reading the docs.
There was a problem hiding this comment.
You're right! I verified from the CPython 3.14 source:
Event.__init__()only doesself._waiters = collections.deque()andself._value = False— no loop access- The
_LoopBoundMixin._get_loop()(which callsevents._get_running_loop()) is only invoked lazily insideEvent.wait()whenself._get_loop().create_future()is called - The
loopparameter was removed in 3.10, and since thenEvent()does lazy loop binding on firstawait
So creating asyncio.Event() outside a running loop is fine on 3.14+. The actual motivation for this refactor is replacing the deprecated new_event_loop() + set_event_loop() + run_until_complete() pattern with asyncio.run() — get_event_loop() raises RuntimeError on 3.14 when no loop is running, and the old manual lifecycle code is replaced by asyncio.run() which handles shutdown_asyncgens() and loop.close() automatically.
Updated the docstrings to reflect the correct motivation in the latest commit.
…ompatibility - TFTP driver: replace new_event_loop()/set_event_loop()/run_until_complete() with asyncio.run(), and move TftpServer construction into async context so asyncio.Event() objects are created with a running loop (required by 3.14) - Shell driver: replace get_event_loop() with get_running_loop() (called from async context, but get_event_loop() is deprecated since 3.10) - mitmproxy example: replace ensure_future() with create_task()
Tests for _start_server, _run_server_lifecycle, and the error handling path to cover the new asyncio.run() based server startup code.
Address review comments on PR jumpstarter-dev#877: 1. Surface startup errors (CodeRabbit): If TftpServer construction fails inside _run_server_lifecycle, the error is now captured in _startup_error and _loop_ready is always set via finally, so start() returns promptly with the real cause instead of blocking for 5s and raising a generic timeout. 2. Correct docstrings (mmahut): asyncio.Event() does NOT require a running loop at construction time on Python 3.14 — the loop is bound lazily on first await via _LoopBoundMixin. Updated docstrings to accurately state the motivation: replacing the deprecated new_event_loop() + set_event_loop() + run_until_complete() pattern with asyncio.run(). 3. Add test for startup error surfacing path.
00444ee to
27b5a4f
Compare
mangelajo
left a comment
mangelajo
left a comment
There was a problem hiding this comment.
Addressed all review comments:
-
CodeRabbit — startup error surfacing: Setup code in
_run_server_lifecycleis now wrapped intry/except/finallyso_loop_ready.set()always fires. Errors are captured in_startup_errorandstart()re-raises them asTftpErrorwith the real cause chained. Addedtest_tftp_start_surfaces_startup_errorto cover this. -
mmahut — asyncio.Event() docstring correction: Verified from CPython 3.14 source that
Event.__init__()does no loop access (lazy binding via_LoopBoundMixin). Updated docstrings to reflect the correct motivation: replacing deprecatednew_event_loop()+set_event_loop()+run_until_complete()patterns. -
Rebased onto current
main(was 17 commits behind).
All 25 TFTP tests pass, lint clean.
| self._loop = asyncio.get_running_loop() | ||
| self.server = TftpServer( | ||
| host=self.host, | ||
| port=self.port, |
There was a problem hiding this comment.
Addressed CodeRabbit's suggestion: setup code is now wrapped in try/except/finally so _loop_ready.set() always fires. If TftpServer() construction fails, the error is captured in _startup_error and start() re-raises it as a TftpError with the real cause chained — no more waiting 5s for a generic timeout.
Added test_tftp_start_surfaces_startup_error to cover this path.
|
|
||
| Uses asyncio.run() instead of the deprecated new_event_loop() + | ||
| set_event_loop() + run_until_complete() pattern, which emits | ||
| DeprecationWarning on Python 3.12/3.13 and breaks on 3.14 | ||
| (get_event_loop() raises RuntimeError when no loop is running). |
There was a problem hiding this comment.
Addressed mmahut's review: the previous docstring incorrectly claimed asyncio.Event() needs a running loop on Python 3.14+. Verified from CPython 3.14 source that Event.__init__() does no loop access — the loop is bound lazily via _LoopBoundMixin._get_loop() on first await.
Updated the docstring to state the correct motivation: replacing the deprecated new_event_loop() + set_event_loop() + run_until_complete() pattern with asyncio.run().
Add --cov and coverage source config to the TFTP package's pyproject.toml so that pytest-cov generates coverage.xml with correct source paths. Without this, diff-cover cannot map the TFTP driver's coverage data to the git diff, making all changed lines appear uncovered.
2 participants
Summary
Audited the entire codebase for deprecated asyncio event loop patterns that emit
DeprecationWarningon Python 3.12/3.13 and will break on Python 3.14. Found and fixed three locations (the SNMP driver had the same issue and was already fixed in #775).Compatibility matrix
get_event_loop()(no running loop)get_running_loop()set_event_loop()asyncio.run()asyncio.Event()(no running loop)async defensure_future()create_task()Changes
1. TFTP driver (HIGH — will crash on 3.14)
jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py_start_server()ran in a thread usingnew_event_loop()+set_event_loop()+run_until_complete(), and constructedTftpServer(which createsasyncio.Event()objects) before the loop was running.Replaced with
asyncio.run()and movedTftpServerconstruction into an async method soasyncio.Event()objects are created with a running loop.2. Shell driver (MEDIUM — deprecation warnings on 3.12/3.13)
jumpstarter-driver-shell/jumpstarter_driver_shell/driver.pyReplaced
asyncio.get_event_loop().time()withasyncio.get_running_loop().time()(called from async context, so trivial swap).3. mitmproxy example (LOW — deprecated but functional)
jumpstarter-driver-mitmproxy/examples/addons/data_stream_websocket.pyReplaced
asyncio.ensure_future()withasyncio.create_task().Audit scope
Searched all Python packages for
get_event_loop,new_event_loop,set_event_loop,run_until_complete,ensure_future, andasyncio.Event()/Queue()/Lock()created outside async contexts. The rest of the codebase already uses the correct modern APIs.