fix: replace deprecated asyncio event loop patterns#877
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 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 Sequence Diagram(s)sequenceDiagram
participant TftpStart
participant ServerThread
participant TftpLifecycle
participant TftpServer
TftpStart->>ServerThread: start server thread
ServerThread->>TftpLifecycle: asyncio.run lifecycle
TftpLifecycle->>TftpServer: construct server
TftpLifecycle-->>TftpStart: signal readiness
TftpLifecycle->>TftpServer: await server lifecycle
TftpStart->>TftpStart: raise TftpError on startup failure
Possibly related PRs
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().
There was a problem hiding this comment.
Thank you for confirming, this was just a nitpick comment really!
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.