Azure / Azure/azure-sdk-for-cpp
WS3: Fix client lifecycle and thread safety
- Dominant language
- C++
- Stars
- 205
- Forks
- 172
- Avg merge
- 1d 15m
- Merged PRs (30d)
- 33
Description
## Summary
The C++ Event Hubs clients carry two data races and no defined behavior after close. `ProducerClient::Close` mutates the sender map without taking the lock that guards it. `Processor::m_isRunning` is a plain `bool` written on one thread and read on another. No public header states a concurrency guarantee, and use after close throws `std::out_of_range` from a map lookup.
Severity tags follow the key in #7252.
## Motivation
The .NET clients document safe concurrent use, close is idempotent, and a closed client throws `EventHubsException` with `FailureReason.ClientClosed` (`sdk/eventhub/Azure.Messaging.EventHubs/src/EventHubsException.cs:227`).
In C++, `ProducerClient::Close` iterates and clears `m_senders` without taking `m_sendersLock` (`src/producer_client.cpp:36-69`), while `EnsureSender` mutates the same map under that lock (`src/producer_client.cpp:196-229`). A concurrent `Send` and `Close` is a data race. `Close` also clears `m_sessions` and `m_connections` unguarded. No closed-state flag exists, so use after close reaches `m_senders.at(...)` and throws `std::out_of_range` (`src/producer_client.cpp:230-234`), which is a standard library error rather than a library error the caller can act on.
`Processor::Start` writes `m_isRunning` after it starts the worker thread, `Stop` writes it from another thread (`src/processor.cpp:64,71`), and the worker reads it in its loop (`src/processor.cpp:100`). The member is a plain `bool` (`inc/azure/messaging/eventhubs/processor.hpp:277`), so that is an unsynchronized read and write.
No public header documents which methods are safe to call concurrently or what `Close` guarantees.
## Proposal
- [ ] Make `ProducerClient::Close` take `m_sendersLock` before it iterates and clears `m_senders`, `m_sessions`, and `m_connections` (`src/producer_client.cpp:36-69,196-229`). **[GA blocker]**
- [ ] Make `Close` idempotent, add a closed-state flag, and make use after close throw `EventHubsException` instead of `std::out_of_range` (`src/producer_client.cpp:230-234`). Apply the same treatment to `ConsumerClient::Close`. Reference: .NET raises `FailureReason.ClientClosed`. **[GA blocker]**
- [ ] Make `Processor::m_isRunning` atomic, and set it before the worker thread starts (`inc/azure/messaging/eventhubs/processor.hpp:277`, `src/processor.cpp:51-77,100`). **[GA blocker]**
- [ ] Document the concurrency contract for every public client type in the headers and the README: which methods are safe to call concurrently, and what `Close` guarantees. **[GA quality bar]**
- [ ] Correct the log text in `ConsumerClient::Close`, which says "Close producer client" (`src/consumer_client.cpp:40`). **[GA quality bar]**
## Validation
- [ ] A thread sanitizer run over a concurrent send and close reports no race.
- [ ] A thread sanitizer run over a processor start and stop reports no race.
- [ ] Calling `Close` twice succeeds.
- [ ] Calling `Send` after `Close` throws `EventHubsException`.
- [ ] Every public client header states its concurrency guarantee.
Contributor guide
Assessment
This issue has not been assessed yet.