Azure / Azure/azure-sdk-for-cpp
[EventHubs] The receiver rebuild backoff does not honor cancellation
- Dominant language
- C++
- Stars
- 205
- Forks
- 172
- Avg merge
- 1d 3h
- Merged PRs (30d)
- 37
Description
## Summary
The receiver rebuild path in `PartitionClient::ReceiveEvents` sleeps for the whole backoff period before it tests the context. A caller that cancels its context during a rebuild storm waits for that whole period before the call returns.
## Motivation
The `recover` lambda calls `std::this_thread::sleep_for(retryAfter)` and it tests the context only after that sleep (`sdk/eventhubs/azure-messaging-eventhubs/src/partition_client.cpp:348-349`). The sleep is one uninterruptible block, so a cancel that arrives at the start of the sleep has no effect until the sleep ends.
`retryAfter` comes from `RetryOperation::ShouldRetry` (`sdk/eventhubs/azure-messaging-eventhubs/src/partition_client.cpp:329-331`). `CalculateExponentialDelay` doubles the delay for each attempt, and it caps the result at `RetryOptions::MaxRetryDelay` (`sdk/eventhubs/azure-messaging-eventhubs/src/retry_operation.cpp:148-173`). So a late rebuild attempt holds the calling thread for the whole cap after the caller asked it to stop.
The send retry loop already solves this problem. `WaitForRetryDelay` sleeps in steps of 100 ms, and it calls `context.ThrowIfCancelled()` in each step (`sdk/eventhubs/azure-messaging-eventhubs/src/retry_operation.cpp:14-32`). `RetryOperation::Execute` uses that helper for every backoff that it takes (`sdk/eventhubs/azure-messaging-eventhubs/src/retry_operation.cpp:92`). The helper sits in an anonymous namespace, so it is file local, and the rebuild path cannot call it.
This path came in with #7328, which is the link reattach part of #7254.
## Proposal
Move `WaitForRetryDelay` and its `CancellationCheckInterval` constant out of the anonymous namespace in `sdk/eventhubs/azure-messaging-eventhubs/src/retry_operation.cpp`. Declare the function in the `_detail` namespace in `sdk/eventhubs/azure-messaging-eventhubs/src/private/retry_operation.hpp`, so both call sites use one implementation.
Replace the `sleep_for` call and the `ThrowIfCancelled` call in the `recover` lambda with a call to that helper. The helper throws `OperationCancelledException` from inside the wait, and the lambda already lets that type through, so the cancel path needs no other change.
Add a test that cancels the context during a rebuild backoff. The test must assert that the call returns in much less time than the backoff period.
Contributor guide
Assessment
This issue has not been assessed yet.