Azure / Azure/azure-sdk-for-cpp

WS3: Fix client lifecycle and thread safety

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

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.