Azure / Azure/azure-sdk-for-cpp
Add two-tier client locking and move the mutable state behind a pimpl
- Dominant language
- C++
- Stars
- 205
- Forks
- 172
- Avg merge
- 1d 15m
- Merged PRs (30d)
- 33
Description
## Summary
The client locking design must change to obey `cpp-design-client-methods-thread-safety`. The change alters the layout of the public client classes, so it breaks the ABI. Do this change before the `1.0.0` tag makes the layout permanent. Move the mutable client state behind a pimpl in the same change. Later locking corrections then keep the ABI.
Parent: #7284. Severity tags follow the key in #7252. The code anchors refer to commit `2de7c583e`.
## Motivation
The three small changes in #7284 stop the undefined behavior. They do not give the guarantee that the guidelines demand. One mutex for each map serializes every operation on the client. Operations on different partitions also wait.
AMQP 1.0 does not permit two messages to interleave their frames on one link. The unit of mutual exclusion for a shared sender link is therefore the full delivery. A lock in the transport that protects one frame cannot do this. `azure-core-amqp` has no such lock in any case. `src/impl/uamqp/amqp/message_sender.cpp` has no mutex. One global polling thread drives the protocol objects (`azure-core-amqp/src/common/global_state.cpp:109`). The Event Hubs layer must do the serialization.
The mutex members are in the public classes today (`inc/azure/messaging/eventhubs/producer_client.hpp:194-201`, `inc/azure/messaging/eventhubs/consumer_client.hpp:206-215`). Their size and their layout are therefore part of the ABI. `azure-core-amqp` is at `1.0.0-beta.12`. The clients still test `ENABLE_UAMQP` and `ENABLE_RUST_AMQP`, so the surface can still change. #7253 (WS1) and #7258 (WS6) cover the related surface work.
## Proposal
Tier 1 protects the client registries.
- [ ] Replace `m_sendersLock`, `m_receiversLock` and `m_sessionsLock` with a `std::shared_mutex`. **[GA blocker]**
- [ ] Take a shared lock to read the map. Take a unique lock to write the map. **[GA blocker]**
- [ ] Keep the link and session creation outside both lock modes. #7286 makes this change. **[GA blocker]**
Tier 2 protects the wire.
- [ ] Give each partition entry its own `std::mutex` in the map value. **[GA blocker]**
- [ ] Keep that mutex for the full `ProducerClient::Send` delivery, because AMQP does not permit interleaving on one link (`src/producer_client.cpp:100-143`). **[GA blocker]**
Structure.
- [ ] Move the mutable client state behind a pimpl (`inc/azure/messaging/eventhubs/producer_client.hpp`, `inc/azure/messaging/eventhubs/consumer_client.hpp`). **[GA blocker]**
- [ ] Do the same for `PartitionClient` and `Processor`. The processor calls them from its own partition threads. **[GA blocker]**
Obey these constraints. Do not use `std::recursive_mutex` again. The recursion stops when the creation moves outside the lock. Continue to return the sender and session handles by value, as the code does today. No reference then stays alive after the lock. Treat `std::shared_ptr` as atomic for its control block only, never for the pointee. Never call a user callback while a lock is locked, because the callback can call back into the client.
One question controls the tier 2 design. `azure-core-amqp` can need single-threaded access, as Qpid Proton does. A work queue for each connection then replaces the mutex for each partition, and deliveries go to that queue. The public contract is the same for both answers, so tier 1 and the documents do not wait for it. Read `MessageSender`, `MessageReceiver`, `Session` and `Connection` in the `ENABLE_UAMQP` backend and the `ENABLE_RUST_AMQP` backend. Then choose.
## Validation
- [ ] Sends to different partitions on one shared client run in parallel. Measure them against a baseline that serializes them.
- [ ] Sends to the same partition serialize. The service receives no interleaved delivery.
- [ ] The thread sanitizer reports no race for the mixed workload.
- [ ] The public client headers show no mutex member.
- [ ] The changelog records the ABI break before `1.0.0`.
- [ ] No user callback runs while a client lock is locked. A code review of the processor and partition client call sites shows this.
Contributor guide
Assessment
This issue has not been assessed yet.