Azure / Azure/azure-sdk-for-rust
[AMQP] Dropping a live AMQP object silently skips the awaited teardown
- Dominant language
- Rust
- Stars
- 884
- Forks
- 365
- Avg merge
- 2d 19h
- Merged PRs (30d)
- 109
Description
## Summary
`azure_core_amqp` gives each AMQP object an awaited teardown method, and it also gives each object a `Drop` implementation that only writes a debug log. When the code drops a live object without a call to the awaited method, the teardown falls back to the best-effort `Drop` of `fe2o3-amqp`, which puts an `End` or `Detach` frame in a queue and does not wait for it. The two paths look the same to the caller, and nothing reports the difference. The unsafe path is the silent default, and the safe path is opt-in.
## Motivation
Each wrapper type has an awaited teardown method. `AmqpSession::end` is at `sdk/core/azure_core_amqp/src/session.rs:73`, `AmqpConnection::close` at `connection.rs:61`, `AmqpSender::detach` at `sender.rs:67`, `AmqpReceiver::detach` at `receiver.rs:89`, `AmqpClaimsBasedSecurity::detach` at `cbs.rs:26`, and `AmqpManagement::detach` at `management.rs:24`. These methods do the correct work. `Fe2o3AmqpSession::end` waits for the session to end and first reads `is_ended()` (`src/fe2o3/session.rs:114-124`).
Each wrapper type also has a `Drop` implementation that writes a debug log and does nothing else. See `src/fe2o3/session.rs:22-26`, `src/fe2o3/management.rs:29-33`, `src/fe2o3/cbs.rs:40-44`, and `src/fe2o3/connection.rs:41-45`. The real teardown then comes from `fe2o3-amqp`. In version 0.14.0, `impl Drop for SessionHandle` is `let _ = self.control.try_send(SessionControl::End(None));` (`src/session/mod.rs:80-84`). The awaited method sends the same control message and then waits for the result (`src/session/mod.rs:132-137`).
The behavior of `fe2o3-amqp` is deliberate, and it is correct for Rust. A `Drop` implementation cannot await, so a best-effort send is the only option there. The crate documents the effect on `SessionHandle` with the line "Dropping the handle will also stop the `Session` event loop", it makes `is_ended()` public, and it points the caller at the awaited method. The change therefore belongs in `azure_core_amqp`, which wraps those handles and hides the difference from its own callers.
Three problems follow.
The teardown does not complete before the drop returns. The caller continues while the peer still holds the link or the session open. Code that drops one object and then creates the next one can therefore break a limit of the service, because the service still counts the first object.
The teardown can be lost. `try_send` returns an error when the channel is full or closed, and the `Drop` implementation discards that error. The object then stays open for the life of the connection.
Neither problem produces a diagnostic. The debug log states that the drop happened. It does not state that the object was still live, so a reader of the logs cannot tell a clean teardown from a lost one.
The Event Hubs SDK found this. Its recoverable connection calls the awaited methods in the recovery path, which shows the intent of the API (`sdk/eventhubs/azure_messaging_eventhubs/src/common/recoverable/connection.rs:270-348`). Its claims-based-security path does not. That path attaches a `$cbs` link for each authorization and then drops it. The service permits one `$cbs` link for each connection and rejects a second attach with `NotAllowed`, which the client treats as not retryable. PR #4895 put those authorizations in sequence behind a lock, but the lock releases in the same tick that the `End` frame goes into the queue, so the fix stays probabilistic. Any crate that builds on `azure_core_amqp` can meet the same class of fault.
## Proposal
Make the difference between the two paths visible, and keep the awaited method as the documented way to tear an object down.
Report a drop of a live object. In each `Drop` implementation, read the state of the object. Write a warning when the object is still live, and keep the debug log when the teardown already happened. The state is available today, because `Fe2o3AmqpSession::end` already reads `is_ended()` before it acts. `Drop` holds `&mut self`, so the code can reach the handle without the async lock.
State the contract in the documentation of each type. Say that `Drop` is best-effort, that it does not wait, and that it can lose the teardown. Say that the caller must call the awaited method to know that the peer released the object.
Do not make `Drop` await the teardown. A `Drop` implementation cannot await, and a background task for the teardown would hide the same race behind a new one.
Consider a debug assertion for a drop of a live object, so a test build fails instead of writing a warning. Make this a separate decision, because it changes the behavior of existing callers.
## Validation
- Add a test that drops a live session and makes sure the warning appears.
- Add a test that ends a session and then drops it, and make sure no warning appears.
- Run the Event Hubs tests, and make sure the new warning does not fire on the recovery path that already calls the awaited methods.
- Count the warnings in an Event Hubs live run. Each one marks a place that needs the awaited teardown, and the claims-based-security path is the first known case.
Contributor guide
Assessment
This issue has not been assessed yet.