Azure / Azure/azure-sdk-for-cpp
[EventHubs] PartitionClient publishes no thread safety contract for a concurrent Close
- Dominant language
- C++
- Stars
- 205
- Forks
- 172
- Avg merge
- 1d 3h
- Merged PRs (30d)
- 37
Description
## Summary
`PartitionClient` documents no thread safety contract. A `Close` that runs at the same time as `ReceiveEvents` on one instance is a data race on `m_receiver`, and it can leave an open receiver attached after `Close` returned.
## Motivation
`m_receiver` is a value wrapper that holds a non-atomic `std::shared_ptr` (`sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/message_receiver.hpp:191`). `Close` reads that pointer at `sdk/eventhubs/azure-messaging-eventhubs/src/partition_client.cpp:272` and `RebuildReceiver` writes it at `:303`, with no synchronization, so concurrent calls are undefined behavior. There is also a window between the closed test at `:412` and the assignment at `:303`: a `Close` that lands inside it lets the rebuild attach a new receiver after `Close` returned. `ReceiveEvents` also mutates `m_lastReceivedOffset` (`:356`) and `m_pendingError` (`:396`, `:480`) with no lock. The exposure predates PR #7337; on `feat/link-reattach` the recover loop rebuilds with no closed test at all. The Azure SDK for C++ guidelines state "DO be thread-safe. All public members of the client type must be safe to call from multiple threads concurrently", and this class does not meet that today. The class doc at `sdk/eventhubs/azure-messaging-eventhubs/inc/azure/messaging/eventhubs/partition_client.hpp:55-59`, the `ConsumerClient` doc, and the Event Hubs README say nothing about threads, so a caller has no stated contract to follow.
## Proposal
Decide the contract first, then make the code match it. Two options are open. The first option documents `PartitionClient` as safe for one thread at a time and states that a caller must not call `Close` while `ReceiveEvents` runs. The second option meets the guideline: move the closed state into the receiver implementation as a latch, or swap the implementation pointer atomically, so `Close` and a rebuild cannot race. A mutex around the closed test and the rebuild does not work, because `Close` would block uninterruptibly while `RebuildReceiver` holds the lock across a network attach, and protecting the receiver reads at `:492` and `:511` would hold the lock across `WaitForIncomingMessage` for the full read deadline.
Found while reviewing #7337, which fixes the sequential case in #7334.
Contributor guide
Assessment
This issue has not been assessed yet.