Azure / Azure/azure-sdk-for-rust
eventhubs: a hung AMQP open blocks recovery and every connection waiter
- Dominant language
- Rust
- Stars
- 884
- Forks
- 365
- Avg merge
- 2d 19h
- Merged PRs (30d)
- 109
Description
## Summary
`ensure_connection` holds the `connections` mutex across the whole AMQP open, and the open has no timeout at any layer. A peer that accepts the socket and then goes silent holds that lock until the process ends. Connection-level recovery cannot run, and every caller that needs the connection waits with it.
## Motivation
`ensure_connection` takes the lock at `sdk/eventhubs/azure_messaging_eventhubs/src/common/recoverable/connection.rs:377` and awaits `create_connection` under it at `:379`. `apply_recovery_plan` takes the same lock at `:799`, in its `drop_connection` step.
This is contention, not a self-deadlock. The guarded region has one await, `create_connection` at `:529`, and that function does not use `recover_azure_operation`, does not call `authorize_path`, and never touches the authorizer. Every `recover_from_error` call site is a retry-wrapper hook. `recover_with_backoff` runs the hook only after the operation returns an error. A nested `ensure_connection` has therefore already released the guard. The distinction matters for severity, and it is fragile. A future change that gives `create_connection` a recovery hook turns this into a true self-deadlock. A change that makes it call `authorize_path` does the same. Both give the shape of #4728.
The open is unbounded. `create_connection` sets no timeout and wraps the call in no `timeout` or `select`. `AmqpConnectionOptions` has no connect-timeout field; its `idle_timeout` governs heartbeats after the connection opens. `Fe2o3AmqpConnection::open` awaits `builder.open(endpoint)` bare. In fe2o3-amqp 0.14.0, `TcpStream::connect` carries no timeout, the header exchange and `ConnectionEngine::open` carry none, and `negotiate_sasl` has a `// TODO: timeout?` above an unbounded read loop. The retry layer does not help. `recover_with_backoff` checks its elapsed budget only after an attempt returns, so it cannot interrupt an attempt that never returns.
A peer that completes the TCP handshake and then stalls during TLS, the AMQP header exchange, or SASL hangs the open with no bound. A blackholed SYN is bounded by OS retry, which is the floor rather than the ceiling.
`drop_connection` runs first in `apply_recovery_plan`, so a stall there blocks the rest of the plan behind it, including `clear_authorizer`, the three cache clears, and `drop_mgmt_client`. Only `ReconnectConnection` sets `drop_connection`. `ReconnectSession` and `ReconnectLink` never take this lock, so those paths are unaffected. The blocked case is the connection-level one, which is the case that most needs to work during a transport failure.
Recovery is not the only caller that waits. `get_session` (`:498`), `ensure_amqp_cbs` (`:604`), `ensure_receiver` (`:635`), and `create_management_client` (`common/recoverable/management.rs:51`) all call `ensure_connection`. One hung open freezes every partition's attach, not only recovery.
## Proposal
Add a timeout around `create_connection` first. It is a small change and it needs no change to the locking. It makes the hang self-terminating, and it helps every waiter rather than only recovery. Give the timeout a documented default and a way for the caller to set it.
Then remove the lock coupling. `connections` can hold the same lock-free cache the sender, session and receiver paths use, that is `RwLock>>>`. `ensure_connection` clones the cell pointer under a brief read lock and runs `get_or_try_init` with no lock held, in the same shape as `or_init_cell` at `connection.rs:105`. `apply_recovery_plan` swaps in a fresh cell under a brief write lock, so it never waits for an open in flight. This keeps both properties the mutex provides now: concurrent opens still share one attempt, and recovery can still replace the connection. It inherits the stale-cell window already documented at `connection.rs:806` and tracked in #4454.
The timeout is the higher priority of the two. The restructure alone removes the lock coupling, but the doomed open still hangs for whichever task awaits it.
Related to #4728 and #4806, which fix the same family of defect on the management-client path.
Contributor guide
Research direction
Start in sdk/eventhubs/azure_messaging_eventhubs/src/common/recoverable/connection.rs at ensure_connection, create_connection, or_init_cell, and apply_recovery_plan; also inspect common/recoverable/management.rs:51 for another waiter. Done means create_connection has a documented, configurable timeout and connection recovery can replace the cell without holding the connections lock across the open.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- azure, rust
- Domain
- backend, distributed-systems, networking
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 48/100