Azure / Azure/azure-sdk-for-cpp

Lock the batch read in EventDataBatch::ToAmqpMessage

Open
#7,287 0 comments 0 reactions 1 assignee Claimed by @j7nw4r View on GitHub
Event Hubs
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.