Repository navigation
fix: close Janus queues during client shutdown - #1258
Merged
Merged
Conversation
Construct Janus 2 queues without an event-loop round trip and close every fixed and dynamic inbox after its consumers stop. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Carter Tinney (cartertinney)
changed the title
Copilot started reviewing on behalf of
Carter Tinney (cartertinney)
September 1, 2026 19:51
View session
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The implementation matches Janus 2 lifecycle semantics and includes focused regression coverage.
Pull request overview
Updates async client shutdown to properly release Janus queue resources.
Changes:
- Uses Janus 2’s loop-independent queue construction.
- Closes fixed and dynamic inboxes after consumers stop.
- Requires Janus 2.x and adds shutdown coverage.
File summaries
| File | Description |
|---|---|
pyproject.toml |
Requires Janus 2.x. |
uv.lock |
Records the Janus version constraint. |
async_inbox.py |
Simplifies construction and adds queue shutdown. |
async_clients.py |
Closes all inboxes during client shutdown. |
inbox_manager.py |
Exposes all managed inboxes. |
test_async_inbox.py |
Tests construction and queue closure. |
test_async_clients.py |
Tests closure of fixed and dynamic inboxes. |
Review details
- Files reviewed: 6/7 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Carter Tinney (cartertinney)
requested review from
Avishek (avishekpant) and
olivakar
September 1, 2026 21:09
Avishek (avishekpant)
approved these changes
Sep 1, 2026
olivakar
approved these changes
Sep 2, 2026
Carter Tinney (cartertinney)
added a commit
that referenced
this pull request
Sep 2, 2026
## Summary - serialize first-time creation of the three shared async client event loops - preserve lock-free access after a loop has been published - synchronize test-only loop cleanup with loop creation - add concurrent initialization and Janus loop-affinity regression coverage ## Root cause [Python E2E build 163209](https://dev.azure.com/azure-iot-sdks/f9b79625-2860-4d92-a4ee-57b03fabfd10/_build/results?buildId=163209) timed out in the first async C2D test on Windows/Python 3.10. Message delivery succeeded, but the log showed `CLIENT_INTERNAL_LOOP` being created twice by concurrent first callers. The loop getters used an unsynchronized check-then-create. After #1258 stopped constructing each Janus queue through the internal loop, the loop was no longer guaranteed to be initialized before the handler runner and feature-enablement paths started concurrently. The queue could bind to the first loop while the second loop replaced the global reference. Every subsequent queue receive then failed Janus's loop-affinity check, and the handler manager's immediate restart behavior turned the failure into a tight restart loop until pytest timed out. ## Why this shape The process-wide loops are intentionally shared by clients to isolate internal work, handler runners, and user coroutine handlers. Moving to per-client loops would require broader ownership and shutdown changes while leaving lazy initialization subject to the same race. Eager creation would unconditionally start three daemon threads. A centralized, double-checked creation lock restores the existing singleton invariant at its owner and keeps the established hot path lock-free. ## Why prior review missed it #1258's reviews and tests focused on Janus resource closure and removing an unnecessary event-loop round trip during queue construction. Its unit tests verified sequential construction, queue operations, and shutdown, and its complete E2E matrix passed. The older loop manager only asserted that repeated sequential calls returned the same loop; it had no concurrent-first-access coverage. The change therefore exposed a pre-existing race rather than introducing an obviously incorrect Janus operation. It requires two independent first consumers to enter a narrow scheduling window, which did not occur in #1258's runs and appeared later in one Windows job. This PR makes that concurrency contract explicit and deterministic in tests. ## Validation - regression demonstrated before the fix: all three loop getters created two loops under synchronized concurrent first access - Python 3.10 focused async lifecycle suite: 153 passed - Python 3.14 related async lifecycle suite: 933 passed, 3 skipped - full unit suite: 5445 passed, 6 skipped - concurrency regression repeated 20 times - package sdist and wheel build succeeded - Black and Ruff passed - independent design and code reviews completed --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Testing
uv run --frozen pytest tests/unit/iothub/aio/test_async_inbox.py tests/unit/iothub/aio/test_async_clients.py tests/unit/iothub/test_inbox_manager.py -W error::RuntimeWarning -q(790 passed, 3 skipped)