Azure / Azure/azure-sdk-for-cpp

Move the Event Hubs link and session creation outside the client locks

Open
#7,286 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

`ProducerClient::EnsureSender` keeps `m_sendersLock` across the link creation and `MessageSender::Open`. `Open` sends data to the service and waits for the answer. The first send to one partition must therefore wait for the link setup of every other partition. This also puts a lock across a call that the AMQP layer identifies as a deadlock risk.

Parent: #7284. Severity tags follow the key in #7252. The code anchors refer to commit `2de7c583e`.

## Motivation

`EnsureSender` takes the lock at `src/producer_client.cpp:229`. It keeps the lock through `GetSession(partitionId).CreateMessageSender(targetUrl, senderOptions)` and `sender.Open(context)`. It writes to the map at `src/producer_client.cpp:256`. `Open` does the AMQP attach handshake, so the lock stays locked for a full network round trip.

`EnsureSession` and `EnsureConnection` have the same design under `m_sessionsLock`. That mutex is a `std::recursive_mutex` (`inc/azure/messaging/eventhubs/producer_client.hpp:197`). The recursion is necessary only because `EnsureSession` calls `EnsureConnection` while it keeps the lock. Move the creation outside the lock, and the recursion stops. The mutex can then be a plain mutex.

The AMQP layer identifies a related risk. `MessageSenderImpl::Open` registers the link with the global polling thread through `AddPollable` (`azure-core-amqp/src/impl/uamqp/amqp/message_sender.cpp:290,340`). The note at `azure-core-amqp/src/common/global_state.cpp:159` says that the caller must not keep a connection lock or a link lock when it calls `AddPollable`. The polling thread takes the connection lock, and the two threads then deadlock.

`m_sendersLock` is an Event Hubs lock, not a link lock. The deadlock in that note therefore does not occur as written. The call stack does nest Event Hubs locks across that boundary. The transport authors identified this pattern as a risk.

## Proposal

- [ ] Change `EnsureSender` to a double-checked write (`src/producer_client.cpp:225-257`). **[GA blocker]**
- [ ] Change `EnsureSession` and `EnsureConnection` in the same way (`src/producer_client.cpp:197-224`). **[GA blocker]**
- [ ] Change the equivalent `ConsumerClient` methods in the same way (`src/consumer_client.cpp:120-165`). **[GA blocker]**
- [ ] Change `m_sessionsLock` from `std::recursive_mutex` to `std::mutex` after the recursion stops (`inc/azure/messaging/eventhubs/producer_client.hpp:197`, `inc/azure/messaging/eventhubs/consumer_client.hpp:210`). **[GA quality bar]**

The double-checked write has four steps. Read the map under the lock. Release the lock. Build the link and call `Open` with no lock. Take the lock again and write to the map after a second read.

Two threads can create a link for the same partition at the same time. One thread wins. The other thread must close its extra link before it returns the handle of the winner. Do not discard the extra link, because that leaks a link.

## Validation

- [ ] No lock stays locked across `Open`, `CreateMessageSender`, or any other call that uses the network. A code review of the four call sites shows this.
- [ ] A test that opens links for N partitions at the same time takes about the time of one link setup.
- [ ] The thread sanitizer reports no race for the double-checked write.
- [ ] Two threads that create the same partition at the same time leak no link.
- [ ] `m_sessionsLock` is a plain `std::mutex`. The 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.