Azure / Azure/azure-sdk-for-rust

eventhubs: a hung AMQP open blocks recovery and every connection waiter

Open
#4,810 1 comment 1 reaction 0 assignees View on GitHub
Client Event Hubs
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.