fix(core-amqp): avoid uAMQP pollable registry deadlock - #7373
Open
Johnathan W (j7nw4r) wants to merge 4 commits into
Open
fix(core-amqp): avoid uAMQP pollable registry deadlock#7373Johnathan W (j7nw4r) wants to merge 4 commits into
Johnathan W (j7nw4r) wants to merge 4 commits into
Conversation
Add a registry progress test for concurrent pollable operations and a completion barrier characterization test.
|
Azure Pipelines: Successfully started running 2 pipeline(s). 8 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes uAMQP pollable-registry deadlocks while preserving removal completion barriers.
Changes:
- Replaces busy-waiting with generation-based condition-variable synchronization.
- Moves sender/receiver registry activation outside connection locks.
- Adds concurrent registry tests and release notes.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
sdk/core/azure-core-amqp/src/common/global_state.cpp |
Implements non-blocking registry synchronization. |
sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/common/global_state.hpp |
Adds generation and condition-variable state. |
sdk/core/azure-core-amqp/src/impl/uamqp/amqp/message_sender.cpp |
Adjusts sender lock scope. |
sdk/core/azure-core-amqp/src/impl/uamqp/amqp/message_receiver.cpp |
Adjusts receiver lock scope and guidance. |
sdk/core/azure-core-amqp/test/ut/global_state_tests.cpp |
Tests concurrent registry operations and barriers. |
sdk/core/azure-core-amqp/test/ut/CMakeLists.txt |
Registers the new tests. |
sdk/core/azure-core-amqp/CHANGELOG.md |
Documents the deadlock fix. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Johnathan W (j7nw4r)
marked this pull request as ready for review
August 25, 2026 15:59
|
Azure Pipelines: Successfully started running 2 pipeline(s). 8 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
The old polling loop hit continue inside a do-while when the registry was empty, which skipped the 100 ms sleep. The thread spun on one core after a process closed its last connection. The condition variable wait fixes that, and the changelog entry for #7370 now says so.
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
GlobalStateHolder::RemovePollable can deadlock when CBS refresh or sender cleanup waits for a poll in progress.
Motivation
RemovePollable held m_pollablesMutex while waiting for an in-flight Poll completion. This blocks unrelated registry operations and can deadlock uAMQP connection or link cleanup paths. Event Hubs CreateBatch cleanup and CBS management refresh share affected lock-order risks.
Changes
Test plan
Affected uAMQP and Event Hubs build passed. Core AMQP suite passed 210 of 210. Playback runner passed 224 tests with 14 live-only skips. Concurrency tests passed 100 repetitions on the first validation pass, 50 on the second, and 20 in the root rerun. clang-format, CSpell, git diff check, commit boundary check, and Rust diff audit passed. Direct CBS and CreateBatch integration tests were not added because unchanged source aborts in an existing TransportImpl handle assertion before their assertions; core concurrency tests and static lock-scope audit cover these paths. The exact live Event Hubs runner was not run because EVENTHUBS_HOST is absent.
Closes #7370