Azure / Azure/azure-sdk-for-cpp
Lock the batch read in EventDataBatch::ToAmqpMessage
- Dominant language
- C++
- Stars
- 205
- Forks
- 172
- Avg merge
- 1d 3h
- Merged PRs (30d)
- 37
Description
## Summary
`EventDataBatch::ToAmqpMessage()` is `const`. `m_rwMutex` is not `mutable`. A `const` method cannot lock a mutex that is not `mutable`, so the method reads the batch state with no lock. `TryAddAmqpMessage` takes that same lock. `ProducerClient::Send` calls `ToAmqpMessage()` first, so a `TryAdd` from a second thread races the serialization.
Parent: #7284. Severity tags follow the key in #7252. The code anchors refer to commit `2de7c583e`.
## Motivation
`EventDataBatch` declares `std::mutex m_rwMutex;` with no `mutable` keyword (`inc/azure/messaging/eventhubs/event_data_batch.hpp:56`). `TryAddAmqpMessage` takes the mutex at `src/event_data_batch.cpp:68`. The header overload takes it at `inc/azure/messaging/eventhubs/event_data_batch.hpp:136`.
`ToAmqpMessage()` is `const` (`inc/azure/messaging/eventhubs/event_data_batch.hpp:145`, defined at `src/event_data_batch.cpp:22`). It therefore reads `m_batchEnvelope`, `m_marshalledMessages` and `m_partitionKey` with no lock. `ProducerClient::Send` calls it at `src/producer_client.cpp:102`, before the retry loop starts.
The type already accounts for the mutex in its copy constructor and its assignment operator. The compiler cannot default them because of the mutex (`inc/azure/messaging/eventhubs/event_data_batch.hpp:73-81`). The unlocked `const` read is the one remaining gap.
The .NET library solves the same problem with a lock and an assert. The producer locks the batch for the full send. A parallel add then throws. The .NET and Python documents both say that the batch type is not thread safe, so the C++ library can also make the caller serialize. A document alone is not sufficient, because the code today corrupts the batch quietly.
The .NET `EventDataBatch` class reference and the .NET README disagree about thread safety. The .NET source gives the answer. The batch takes an internal lock, and wrong use throws.
## Proposal
- [ ] Make `m_rwMutex` `mutable`. Take it in `ToAmqpMessage()` (`inc/azure/messaging/eventhubs/event_data_batch.hpp:56`, `src/event_data_batch.cpp:22`). **[GA blocker]**
- [ ] Add a locked flag. `ProducerClient::Send` sets the flag for the full delivery (`src/producer_client.cpp:100-143`). **[GA blocker]**
- [ ] Make a parallel `TryAdd` return `false` or throw `EventHubsException` while the flag is set. **[GA blocker]**
- [ ] Write the thread safety contract in the `EventDataBatch` header. **[GA quality bar]**
Use the .NET words for the contract. The type is not thread safe. A caller must not share one batch across threads. A caller must not use a batch while the client sends it.
The error from the parallel `TryAdd` must be an `EventHubsException`. It must not be a `std::runtime_error`. #7273 (WS10) covers the full error model.
## Validation
- [ ] The thread sanitizer reports no race for a `Send` on one thread and a `TryAdd` on a second thread against the same batch.
- [ ] A `TryAdd` during a `Send` fails each time.
- [ ] The batch that the service receives is correct.
- [ ] The `EventDataBatch` header gives the thread safety contract.
- [ ] The batch unit tests pass.
Contributor guide
Assessment
This issue has not been assessed yet.