| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
Merging this PR will not alter performance✅ 6 untouched benchmarks Comparing bluetoothbot:koan/use-asyncio-kwarg (86b950f) with master (9138f32) |
Sorry, something went wrong.
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## master #1684 +/- ##
=======================================
Coverage 99.76% 99.76%
=======================================
Files 33 33
Lines 3407 3410 +3
Branches 463 464 +1
=======================================
+ Hits 3399 3402 +3
Misses 5 5
Partials 3 3 ☔ View full report in Codecov by Harness.
|
Sorry, something went wrong.
There was a problem hiding this comment.
Adds an explicit use_asyncio: bool | None = None switch to zeroconf.Zeroconf to override the current auto-detection of a running asyncio event loop (notably to support forcing thread-based/blocking semantics in environments like Jupyter where a loop is always running).
Changes:
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/zeroconf/_core.py | Adds use_asyncio kwarg and consults it during startup to choose between attaching to a running loop vs starting the internal thread/loop. |
| tests/test_core.py | Adds tests validating the new use_asyncio behaviors and preserving the default auto-detect behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sorry, something went wrong.
Sorry, something went wrong.
PR Review — feat(core): add use_asyncio kwarg to ZeroconfSmall, well-scoped feature addition that solves a real Jupyter-style problem. The implementation correctly places the use_asyncio=True precondition check before create_sockets(), so the resource-leak concern Copilot raised against an earlier revision is already addressed. Default behavior (use_asyncio=None) is preserved, so this is backwards-compatible. Two minor changes worth applying before merge: (1) take @bdraco's ternary suggestion in start(), (2) add a happy-path test for use_asyncio=True with a running loop to lock in the contract. Both are small and don't block merge on their own. 🟢 Suggestions1. Collapse if/else into a ternary (`src/zeroconf/_core.py`, L232-235)Per @bdraco's review comment (id 3253356037), the four-line if/else in start() can be a single line: self.loop = None if self._use_asyncio is False else get_running_loop()It's equivalent and keeps the body of start() flatter. Note the explicit is False is correct here — None (the default) must still fall through to get_running_loop(), so if not self._use_asyncio would be wrong. if self._use_asyncio is False:
self.loop = None
else:
self.loop = get_running_loop()
The three new tests cover use_asyncio=False (with loop), use_asyncio=True (no loop → error), and use_asyncio=None (no loop). The fourth combination — use_asyncio=True with a running loop — is the documented success path but isn't pinned. Functionally it's the same as None in that case, but a regression could silently swap them. Suggest adding: def test_use_asyncio_true_with_running_loop_attaches(self):
async def run() -> r.Zeroconf:
return r.Zeroconf(interfaces=["127.0.0.1"], use_asyncio=True)
loop = asyncio.new_event_loop()
zc: r.Zeroconf | None = None
try:
zc = loop.run_until_complete(run())
assert zc._loop_thread is None
assert zc.loop is loop
finally:
if zc is not None:
zc.close()
loop.close()The use_asyncio=True precondition check is placed after the apple_p2p platform check but before create_sockets(). Placement before sockets correctly addresses Copilot's resource-leak concern. As a small polish, hoisting it above the apple_p2p block keeps all kwarg-precondition checks grouped at the top of __init__ before any side effects. Non-blocking. if use_asyncio is True and get_running_loop() is None:
raise RuntimeError("use_asyncio=True requires a running asyncio event loop")
Checklist
SummarySmall, well-scoped feature addition that solves a real Jupyter-style problem. The implementation correctly places the use_asyncio=True precondition check before create_sockets(), so the resource-leak concern Copilot raised against an earlier revision is already addressed. Default behavior (use_asyncio=None) is preserved, so this is backwards-compatible. Two minor changes worth applying before merge: (1) take @bdraco's ternary suggestion in start(), (2) add a happy-path test for use_asyncio=True with a running loop to lock in the contract. Both are small and don't block merge on their own. |
Sorry, something went wrong.
Rebase with requested adjustmentsBranch koan/use-asyncio-kwarg was rebased onto master and review feedback was applied. Changes applied
Stats2 files changed, 46 insertions(+), 1 deletion(-)
CI statusCI will be checked asynchronously. Automated by Kōan |
Sorry, something went wrong.
Sorry, something went wrong.
Rebase with requested adjustmentsBranch koan/use-asyncio-kwarg was rebased onto master and review feedback was applied. Stats2 files changed, 46 insertions(+), 1 deletion(-)
CI statusCI will be checked asynchronously. Automated by Kōan |
Sorry, something went wrong.
Sorry, something went wrong.
Rebase with requested adjustmentsBranch koan/use-asyncio-kwarg was rebased onto master and review feedback was applied. Changes applied
Stats2 files changed, 47 insertions(+), 1 deletion(-)
CI statusCI will be checked asynchronously. Automated by Kōan |
Sorry, something went wrong.
Sorry, something went wrong.
Zeroconf currently auto-detects a running event loop and attaches to it; passing use_asyncio=False forces the historic thread-mode path even when a loop is running (e.g. Jupyter). use_asyncio=True requires a running loop and raises otherwise. None (default) preserves the existing behaviour. Closes python-zeroconf#1651
Rebase with requested adjustmentsBranch koan/use-asyncio-kwarg was rebased onto master and review feedback was applied. Changes applied
Stats2 files changed, 44 insertions(+), 1 deletion(-)
CI statusCI will be checked asynchronously. Automated by Kōan |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
What
Add use_asyncio: bool | None = None to Zeroconf.__init__.
Why
Zeroconf auto-detects a running event loop via get_running_loop() and attaches to it. In environments like Jupyter that always have an event loop running to support top-level await, this makes it impossible to use the blocking thread-mode API for code that is otherwise sync. Issue #1651 asked for an explicit override.
How
The flag is stored as self._use_asyncio and consulted in start() before calling get_running_loop(). No changes to AsyncZeroconf — it implicitly needs a loop and the existing behaviour is correct there.
Testing
Three new unit tests in tests/test_core.py:
poetry run pytest tests/test_core.py → 24 passed, 1 skipped.
Closes #1651
Quality Report
Changes: 2 files changed, 45 insertions(+), 1 deletion(-)
Code scan: clean
Tests: failed (timeout (120s))
Branch hygiene: clean
Generated by Kōan post-mission quality pipeline