Catch exceptions in EventHubs client destructors#7116
Conversation
Wrap the Close() calls in ProducerClient, ConsumerClient, and PartitionClient destructors with try/catch so a throw from Close() no longer terminates the program via std::terminate. Caught exceptions are logged at Warning level. Each Log::Stream call is itself wrapped so a logger allocation failure or a throwing user listener cannot escape the noexcept destructor. Fixes Azure#6957
There was a problem hiding this comment.
Pull request overview
This PR hardens Azure Event Hubs client cleanup by ensuring that exceptions thrown during Close() (or m_receiver.Close()) do not escape noexcept destructors and trigger std::terminate, addressing the scenario described in #6957.
Changes:
- Wrap
Close()/m_receiver.Close()inProducerClient,ConsumerClient, andPartitionClientdestructors withtry/catchand log failures atWarninglevel. - Explicitly mark the three destructors as
noexceptto document/lock in the non-throwing contract. - Move
ProducerClient’s destructor definition out-of-line intoproducer_client.cpp, removing the inlineClose()call from the public header.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk/eventhubs/azure-messaging-eventhubs/src/producer_client.cpp | Adds a noexcept destructor that catches and logs exceptions from Close(). |
| sdk/eventhubs/azure-messaging-eventhubs/src/consumer_client.cpp | Makes destructor noexcept and guards Close({}) with exception handling + warning logs. |
| sdk/eventhubs/azure-messaging-eventhubs/src/partition_client.cpp | Makes destructor noexcept and guards receiver close + logging against exceptions. |
| sdk/eventhubs/azure-messaging-eventhubs/inc/azure/messaging/eventhubs/producer_client.hpp | Replaces inline destructor with an out-of-line noexcept declaration. |
| sdk/eventhubs/azure-messaging-eventhubs/inc/azure/messaging/eventhubs/consumer_client.hpp | Marks destructor noexcept (declaration change). |
| sdk/eventhubs/azure-messaging-eventhubs/inc/azure/messaging/eventhubs/partition_client.hpp | Marks virtual destructor noexcept (declaration change). |
| sdk/eventhubs/azure-messaging-eventhubs/CHANGELOG.md | Documents the bug fix under “Bugs Fixed” with issue link. |
| try | ||
| { | ||
| Close(); | ||
| } | ||
| catch (std::exception const& e) | ||
| { | ||
| try | ||
| { | ||
| Log::Stream(Logger::Level::Warning) | ||
| << "~ProducerClient(): exception thrown during Close(): " << e.what(); | ||
| } | ||
| catch (...) | ||
| { | ||
| } | ||
| } | ||
| catch (...) | ||
| { | ||
| try | ||
| { | ||
| Log::Stream(Logger::Level::Warning) | ||
| << "~ProducerClient(): unknown exception thrown during Close()."; | ||
| } | ||
| catch (...) | ||
| { | ||
| } |
| catch (std::exception const& e) | ||
| { | ||
| try | ||
| { | ||
| Log::Stream(Logger::Level::Warning) | ||
| << "~ConsumerClient(): exception thrown during Close(): " << e.what(); | ||
| } | ||
| catch (...) | ||
| { | ||
| } | ||
| } |
| catch (std::exception const& e) | ||
| { | ||
| try | ||
| { | ||
| Log::Stream(Logger::Level::Warning) | ||
| << "~PartitionClient(): exception thrown during m_receiver.Close(): " << e.what(); | ||
| } | ||
| catch (...) | ||
| { | ||
| } | ||
| } |
Refactor ProducerClient::Close and ConsumerClient::Close so a throw
from one teardown step no longer skips the rest. Each step (properties
client, sender/receiver loop, session loop, connection loop) runs in
its own try/catch. The first exception is captured and rethrown to the
caller after every step has been attempted; subsequent throws are
logged and swallowed.
Also fix two copy-paste log message typos in ConsumerClient::Close
("Close producer client." and "Closing message senders.") and replace
the equivalent-but-cryptic `while(!empty) erase(begin)` drain pattern
with `clear()` to match ProducerClient.
|
Thank you @j7nw4r, we will fix this in #7118 and will release a |
Summary
Two related fixes for cleanup behavior in the EventHubs clients.
1. Catch exceptions in destructors (
Fixes #6957)ProducerClient::~ProducerClient,ConsumerClient::~ConsumerClient, andPartitionClient::~PartitionClienteach callClose()(orm_receiver.Close()) without exception handling. Destructors are implicitlynoexceptin C++11+, so a throw from cleanup invokesstd::terminateand kills the program with no chance to recover.
Close()call in each of the three destructors withtry/catch (std::exception const&)/catch (...); log atWarningand neverrethrow.
noexceptto document the contract and letclang-tidy bugprone-exception-escapeflag future regressions.~ProducerClientout-of-line intoproducer_client.cppso the public header no longer pulls in the internal logging header.Log::Streamcall inside the catch handlers is wrapped in its own innertry/catch (...).Log::Streamallocates astd::stringstreamand its destructor invokes the user-installed log listener, so it can throw and would otherwise re-trigger
std::terminatefrom inside thenoexceptdestructor frame.2. Best-effort teardown in
Close()Previously, if the first step in
ProducerClient::Close/ConsumerClient::Closethrew, every subsequent step was skipped — leaking senders,sessions, and connections.
Close()methods so each teardown step (properties client, sender/receiver loop, session loop, connection loop) runs in its owntry/catch.
std::exception_ptrand rethrown to the caller after every step has been attempted; laterexceptions are logged and swallowed.
Close()updated to document the best-effort contract.Drive-by cleanup
ConsumerClient::Closelog messages:"Close producer client."→"Close consumer client.","Closing message senders."→"Closing message receivers.".while (!m_sessions.empty()) m_sessions.erase(begin)drain pattern inConsumerClient::Closewithclear(), matchingProducerClient.Test plan
main, insertthrow std::runtime_error("forced");at the top ofProducerClient::Close, construct + destroy aProducerClient. Expected: process aborts withterminate called after throwing....throwin place. Expected: clean exit;Warning-level log emitted(with a registered listener).
main, populatem_sendersthen makem_propertiesClient->Close()throw.Expected: senders never get
.Close()called, leaking AMQP state..Close(), the original properties-client exception isrethrown to the caller, intermediate failures are logged.
ConsumerClient.azure-messaging-eventhubs-testnon-live unit tests pass — they exercise both Close paths through happy-path destruction and verify noregression.
No automated regression test added:
ProducerClient/ConsumerClient/PartitionClienthold concreteAzure::Core::Amqp::_internal::MessageSender/MessageReceiver/Session/Connectioninstances by value with no mockable seam. ForcingClose()to throw deterministically from a test would require introducing a mock AMQP interface — a much larger change than this fix.
Breaking changes
None for callers who don't call
Close()explicitly. For callers who do: if the first step ofClose()throws, the same exception is stillrethrown, but subsequent steps now also run (and may log additional warnings) before the rethrow. This is a strict improvement in cleanup
guarantees; no API or signature change.