fix(core): release sockets when close runs before engine setup completes#1706
Conversation
When ``Zeroconf`` is constructed inside a running asyncio loop, the engine's socket→transport wrapping happens in a scheduled task. If the caller closed the instance before that task ran (e.g. an error path that bails out immediately, or a sync ``zc.close()`` called from the same loop before any await), ``_async_shutdown`` had no transports to close and the raw sockets passed to the engine in ``__init__`` leaked their FDs until interpreter shutdown. ``_async_create_endpoints`` now releases its handle on each socket as the transport adopts it, and ``_async_shutdown`` cancels any pending setup task and closes whatever sockets were never adopted. The async close path tolerates a cancelled setup task instead of asserting on ``_cleanup_timer`` that was never scheduled. Also clean up the test-side leaks surfaced by issue #1133: add the missing ``async_close`` / ``close`` calls in tests that constructed a ``Zeroconf`` or ``AsyncZeroconf`` and never tore it down, give ``test_shutdown_loop`` an explicit ``loop.close()``, and add a no-op ``update_service`` to the ``ServiceListener`` subclass in ``test_service_browser_listeners_no_update_service`` so the browser thread doesn't surface a ``NotImplementedError``. Closes #1133.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1706 +/- ##
=======================================
Coverage 99.76% 99.76%
=======================================
Files 33 33
Lines 3410 3426 +16
Branches 464 471 +7
=======================================
+ Hits 3402 3418 +16
Misses 5 5
Partials 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@bluetoothbot review |
PR Review — fix(core): release sockets when close runs before engine setup completesSolid, well-targeted fix for a real FD leak. The core insight — that 🟢 Suggestions1. Pin sender invariant too (`tests/test_engine.py`, L87-92)The test pins 2. Ownership-transfer ordering is fragile under future refactors (`src/zeroconf/_engine.py`, L117-124)The ownership-transfer block (drop from 3. Idempotency docstring overlooks `running_future` replacement (`src/zeroconf/_engine.py`, L165-174)The new docstring says Checklist
SummarySolid, well-targeted fix for a real FD leak. The core insight — that |
|
right |
- Switch ``_async_close`` from ``contextlib.suppress(CancelledError)`` to ``asyncio.gather(setup_task, return_exceptions=True)``. Gather captures a setup task that was cancelled internally while still propagating outer-task cancellation, removing the ambiguity the suppress form had. - Note in ``_async_shutdown``'s docstring that it must stay idempotent — the body is already written that way, but future edits could silently regress it. - Add ``test_setup_releases_socket_ownership`` as a positive-path pin for the new invariant: after a normal start, ``_listen_socket`` is ``None`` and ``_respond_sockets`` is empty.
|
@bluetoothbot review |
There was a problem hiding this comment.
Pull request overview
This PR fixes a file-descriptor leak in AsyncEngine when a Zeroconf instance is closed before asynchronous engine setup finishes (particularly when constructed inside a running asyncio loop). It also updates several tests to consistently close AsyncZeroconf/Zeroconf instances and event loops to eliminate pytest warning noise (issue #1133).
Changes:
- Track socket ownership transfer during
_async_create_endpoints()so shutdown can safely close only “unadopted” sockets. - Make
_async_shutdown()cancel any pending setup task and close any remaining raw sockets not adopted by transports. - Test suite cleanup: ensure
async_close()/close()/loop.close()are called where previously missing, and avoidNotImplementedErrorsurfacing from a browser listener thread.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/zeroconf/_engine.py | Cancel pending setup on shutdown and explicitly close any sockets that never became transports; adjust async close flow. |
| tests/test_engine.py | Adds a regression test asserting engine socket-ownership invariants after startup. |
| tests/test_core.py | Ensures AsyncZeroconf is closed in test_event_loop_blocked even on exceptions. |
| tests/test_asyncio.py | Closes the sync Zeroconf instance in an async test to prevent leaks/warnings. |
| tests/test_handlers.py | Ensures AsyncZeroconf is closed in test_response_aggregation_timings_multiple. |
| tests/services/test_info.py | Closes AsyncZeroconf in several service info async tests. |
| tests/services/test_browser.py | Adds a no-op update_service to avoid thread-surfaced NotImplementedError during the test. |
| tests/utils/test_asyncio.py | Closes the event loop in test_shutdown_loop after stopping it. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Copilot review on #1706: ``gather(return_exceptions=True)`` was too broad — it would have hidden a genuine setup failure (e.g. a socket bind error in ``_async_create_endpoints``) during close. Switch back to ``try/await/except CancelledError`` and gate the swallow on ``self._setup_task.cancelled()``; that's True only when the task itself was cancelled (the close-before-start case we want to absorb), so outer-task cancellation and non-cancel setup exceptions both propagate. Also update the ``test_service_browser_listeners_no_update_service`` docstring to describe what the test now actually asserts — that a listener which ignores update events records only add/remove callbacks — since the listener now provides an explicit no-op ``update_service``.
…r outer-cancel path Address the three follow-up review points on #1706: - Reorder ``_async_create_endpoints`` so the wrapped transport is registered in ``self.readers``/``self.senders`` *before* the engine's handle on the socket is dropped from ``_listen_socket`` / ``_respond_sockets``. The block between the two awaits is atomic under asyncio today, but a future refactor that introduces an ``await`` in the middle would otherwise open a window where the socket is in neither collection and ``_async_shutdown`` would miss it; with the new ordering, briefly being in both is the worst case, and both close paths are idempotent. - Narrow the ``_async_shutdown`` docstring: the setup-task cancel, transport closes, and socket closes are no-ops on the second pass, but ``running_future`` is intentionally replaced each time — call that out so a future reader doesn't try to "fix" the apparent inconsistency. - ``test_setup_releases_socket_ownership`` now also asserts ``engine.senders`` is non-empty; a regression in the sender-wrapping branch would now fail this test instead of silently shipping. - Add ``test_async_close_propagates_outer_cancellation`` to cover the ``raise`` in the ``self._setup_task.cancelled()`` gate. A future whose exception was set to ``CancelledError`` re-raises on ``await`` while still reporting ``cancelled() is False`` — the exact shape of an outer-task cancel at the gate's point of view — so the test pins that outer cancellation surfaces to the caller (engine coverage now 100% with --cov-branch).
|
@bluetoothbot review |
CLAUDE.md is explicit that comments and docstrings default to terse and exist only when the *why* is non-obvious. The previous review rounds left several multi-line comment blocks and docstrings that restated the surrounding code; pare them back to the one-line form the rest of the module uses.
Summary
Closes #1133 (long-standing pytest warning noise).
The library change is the load-bearing one: when
Zeroconfis constructed inside a running asyncio loop,AsyncEngine._async_setupruns as a scheduled task and_async_create_endpointsonly wraps the raw sockets it received in__init__once the loop gives the task a turn. If something closed the instance before that turn — a synczc.close()from the same loop, an error path that bails before the firstawait, or a test that just races —_async_shutdownhad no transports to close, the raw sockets stayed in_listen_socket/_respond_sockets, and the FDs leaked until interpreter shutdown.The rest of the diff is the test-side cleanup #1133 originally pointed at.
Details
_async_create_endpointsnow drops its handle on each socket as the transport adopts it (self._listen_socket = None,self._respond_sockets.remove(s)), so by definition anything left in those collections is unowned._async_shutdowncancels any still-pending setup task (so it can't race in afterward to wrap a socket we're about to free) and then closes whatever wasn't adopted._async_closeswallowsCancelledErrorfrom the awaited setup task and no longer asserts on_cleanup_timer— that timer is only scheduled inside_async_setup, so a setup that was cancelled before its first sync line never sets it.tests/services/test_info.py— four tests created anAsyncZeroconfand never calledasync_close; added it.tests/test_asyncio.py::test_async_service_registration_name_strict_check— same, for a syncZeroconf.tests/test_handlers.py::test_response_aggregation_timings_multiple— same.tests/test_core.py::test_event_loop_blocked— mocks_async_setup, so the engine sockets were never wrapped;async_closenow closes them via the path above.tests/utils/test_asyncio.py::test_shutdown_loop— added the missingloop.close().tests/services/test_browser.py::test_service_browser_listeners_no_update_service— gave the listener an emptyupdate_serviceso the browser thread stops surfacingNotImplementedError(the test's intent — that no update callbacks are recorded — is preserved).No protocol-affecting changes; nothing in
const.pymoved.Test plan
poetry run pytest --timeout=60 tests— 335 passed, 3 skipped, 0 warnings.poetry run pytest -W error::ResourceWarning -W error::pytest.PytestUnraisableExceptionWarning -W error::pytest.PytestUnhandledThreadExceptionWarning --timeout=60 tests— same result; the strict filters that would have errored before now pass.poetry run pre-commit run --files <changed files>— clean.