fix(eventhubs): rethrow last exception when retries are exhausted#7131
Open
j7nw4r wants to merge 1 commit into
Open
fix(eventhubs): rethrow last exception when retries are exhausted#7131j7nw4r wants to merge 1 commit into
j7nw4r wants to merge 1 commit into
Conversation
RetryOperation::Execute returned false instead of rethrowing the
captured exception when every retry attempt threw. WasLastAttempt
is checked against retryCount before the catch increments it, so
the value never reaches MaxRetries inside the catch and the
"else { throw; }" branch is unreachable. ProducerClient::Send
ignored the return value, so a fully failed Send appeared to
succeed and silently dropped the batch.
Capture std::current_exception() in each catch, clear it when a
later attempt completes without throwing, and rethrow it when the
retry loop terminates. As defense in depth, ProducerClient::Send
now throws when Execute reports failure.
Fixes Azure#7130.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a reliability issue in azure-messaging-eventhubs where RetryOperation::Execute() could exhaust retries and then silently return false, causing ProducerClient::Send() to drop a batch without surfacing the final failure (issue #7130).
Changes:
- Track and rethrow the most recent exception in
RetryOperation::Execute()when the retry loop terminates due to exhaustion. - Add a defense-in-depth check in
ProducerClient::Send()to throw ifRetryOperation::Execute()ever reports failure via afalsereturn. - Add regression unit tests for retry exhaustion behavior (EventHubsException and std::exception paths) and for transient-failure recovery behavior; add a CHANGELOG entry.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| sdk/eventhubs/azure-messaging-eventhubs/test/ut/retry_operation_test.cpp | Adds regression tests to validate exception rethrow on retry exhaustion and correct behavior after transient failures. |
| sdk/eventhubs/azure-messaging-eventhubs/src/retry_operation.cpp | Captures the last thrown exception and rethrows it after the retry loop ends. |
| sdk/eventhubs/azure-messaging-eventhubs/src/producer_client.cpp | Adds a fallback exception if Execute() returns false so sends can’t silently fail. |
| sdk/eventhubs/azure-messaging-eventhubs/CHANGELOG.md | Documents the bug fix under the current Unreleased section. |
Comment on lines
31
to
35
| int retryCount = 0; | ||
| // Capture the most recent exception so a retries-exhausted fallthrough can | ||
| // rethrow it instead of silently returning false. See issue #7130. | ||
| std::exception_ptr lastException; | ||
| while (retryCount < m_retryOptions.MaxRetries) |
Comment on lines
39
to
47
| try | ||
| { | ||
| bool result = operation(); | ||
|
|
||
| if (ShouldRetry(result, retryCount, retryAfter)) | ||
| { | ||
| lastException = nullptr; | ||
| retryCount++; | ||
| std::this_thread::sleep_for(retryAfter); |
Comment on lines
+130
to
+131
| throw Azure::Messaging::EventHubs::EventHubsException( | ||
| "ProducerClient::Send failed after exhausting all retry attempts."); |
Comment on lines
+165
to
+183
| auto opts = LocalTest::MakeFastRetryOptions(3); | ||
| Azure::Messaging::EventHubs::_detail::RetryOperation retryOp(opts); | ||
|
|
||
| int callCount = 0; | ||
| auto throwsThenReturnsFalse = [&callCount]() -> bool { | ||
| ++callCount; | ||
| if (callCount == 1) | ||
| { | ||
| throw Azure::Messaging::EventHubs::EventHubsException("first attempt fails"); | ||
| } | ||
| return false; | ||
| }; | ||
|
|
||
| // Second attempt returns false cleanly. ShouldRetry(false=response, retryCount=1) | ||
| // returns true, so the loop retries; on attempt 3 the loop terminates and Execute | ||
| // returns false. The exception from attempt 1 must not be rethrown. | ||
| EXPECT_NO_THROW({ EXPECT_FALSE(retryOp.Execute(throwsThenReturnsFalse)); }); | ||
| EXPECT_EQ(opts.MaxRetries, callCount); | ||
| } |
Member
Author
|
/azp run cpp - eventhubs - ci |
|
Azure Pipelines successfully started running 1 pipeline(s). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #7130.
RetryOperation::Executesilently swallowed the final exception when every retry attempt ofProducerClient::Sendthrew:WasLastAttemptis checked againstretryCountbefore the catch increments it, so the value never reachesMaxRetriesinside the catch and theelse { throw; }branch is unreachable. Control falls through toreturn false;, andProducerClient::Sendignores that value — the batch is dropped without any error surfaced to the caller.This change:
std::current_exception()in each catch block ofRetryOperation::Execute.falsereturn does not surface a stale exception.ProducerClient::Send: ifExecuteever does returnfalse, throw anEventHubsExceptioninstead of dropping the batch silently.EventHubsExceptionafter exhausted retries, rethrow ofstd::exception, immediate throw on fatal exceptions (no retry), success after a transient exception, and the cleared-stale-exception case where a later attempt returnsfalsecleanly.1.0.0-beta.12Unreleased.Test plan
cmake --build build --target azure-messaging-eventhubs-testsucceeds.azure-messaging-eventhubs-test --gtest_filter=RetryOperationTest.*passes (11/11).RetryOperation::Executeand the new defense-in-depth branch inProducerClient::Send.