WS10: Align the Event Hubs error model with the C++ guidelines

[j7nw4r]

Summary

The library throws std::runtime_error in 24 places outside the test code. The C++ guidelines forbid a throw of any exception type that the Azure C++ Core library does not define. EventHubsException also derives from std::runtime_error, and not from an Azure Core exception type. Three of the 24 throws are inline in installed public headers. No public function documents the exceptions that it can throw.

Some of the 24 throws report a caller mistake. The guidelines classify a caller mistake as a pre-condition violation. They require an abort for that case, and they forbid a throw.

The base class change and the pre-condition changes are breaking changes. They must land before 1.0.0. Severity tags follow the key in #7252. All code anchors refer to commit f1039fe23.

Motivation

The rules

The C++ guidelines state five rules for this area:

• cpp-design-logical-errorhandling-exceptions-other (MUST NOT): "throw exceptions, except those from the Azure C++ Core library as described in the error handling section."
• cpp-design-logical-errorhandling-recov-reporting (MUST): "report errors by throwing C++ exceptions defined in the Azure C++ Core Library."
• cpp-design-logical-errorhandling-prec-crash (MUST): "crash, if possible. This means calling some form of fast failing function, like abort."
• cpp-design-logical-errorhandling-prec-exceptions (MUST NOT): "throw a C++ exception."
• cpp-design-logical-errorhandling-recov-document (MUST): "document all exceptions each function and its transitive dependencies may throw, except for std::bad_alloc."

One caution for the review. The example code below cpp-design-logical-errorhandling-recov-reporting throws std::runtime_error, and that example contradicts the rule above it. The two MUST NOT rules carry this issue, because their text is unambiguous.

The library throws a non-Core exception type in 24 places

A search for throw std:: across both packages, and outside the test code, finds 24 sites. Each site throws std::runtime_error.

File	Lines
azure-messaging-eventhubs/src/producer_client.cpp	120, 134
azure-messaging-eventhubs/src/partition_client.cpp	65, 74, 83, 91
azure-messaging-eventhubs/src/checkpoint_store.cpp	16, 27, 41, 55
azure-messaging-eventhubs/src/private/eventhubs_utilities.hpp	142, 148, 196, 202, 257
azure-messaging-eventhubs/src/processor_partition_client.cpp	71
azure-messaging-eventhubs/src/processor_load_balancer.cpp	267
azure-messaging-eventhubs/src/event_data_batch.cpp	27
azure-messaging-eventhubs/inc/azure/messaging/eventhubs/event_data_batch.hpp	184
azure-messaging-eventhubs/inc/azure/messaging/eventhubs/processor.hpp	154, 321
azure-messaging-eventhubs-checkpointstore-blob/src/blob_checkpoint_store.cpp	22, 29, 42

Three throws are inline in installed public headers

event_data_batch.hpp:184, processor.hpp:154, and processor.hpp:321 each throw from an installed header. That code compiles into the translation unit of the caller. The exception type is therefore part of the observed behavior of a build that the team does not control.

Some throws report a caller mistake

The guidelines classify a caller mistake as a pre-condition violation, and a pre-condition violation must abort. Three examples show the pattern:

• event_data_batch.hpp:184 throws for "Either PartitionID or PartitionKey can be set, but not both."
• processor.hpp:154 throws for "cannot close a processor that is running".
• processor_load_balancer.cpp:267 throws for "unknown strategy".

Azure Core supplies the mechanism for this case. AZURE_ASSERT and AZURE_ASSERT_MSG are in sdk/core/azure-core/inc/azure/core/azure_assert.hpp:30,39. The Event Hubs code uses neither macro today.

EventHubsException does not derive from an Azure Core exception

inc/azure/messaging/eventhubs/eventhubs_exception.hpp:19 declares class EventHubsException final : public std::runtime_error. The Storage library uses a different shape: struct StorageException final : public Azure::Core::RequestFailedException (sdk/storage/azure-storage-common/inc/azure/storage/common/storage_exception.hpp:19). Azure::Core::RequestFailedException itself derives from std::runtime_error (sdk/core/azure-core/inc/azure/core/exception.hpp:57), so the Storage shape keeps the standard base class and adds the Core contract.

RequestFailedException does not fit Event Hubs. It carries HTTP fields: RawResponse, StatusCode, ReasonPhrase, ClientRequestId, and RequestId (exception.hpp:63-98). Event Hubs operates over AMQP, so each of those fields stays empty forever on an Event Hubs exception.

The recorded decision: add a new Azure Core exception type for a non-HTTP protocol, and derive EventHubsException from that type.

Three points support the decision:

1. Azure Core already holds exception types that do not derive from RequestFailedException. OperationCancelledException derives from std::runtime_error (sdk/core/azure-core/inc/azure/core/context.hpp:32), and AuthenticationException derives from std::exception (sdk/core/azure-core/inc/azure/core/credentials/credentials.hpp:139). A new sibling type fits the existing shape of the library.
2. A new type in Azure Core is an addition. It keeps each existing consumer of Azure Core compiling.
3. The alternative re-parents RequestFailedException below the new type. That change alters the layout of a type that Azure Core already shipped, so this issue does not propose it.

The name needs the API review. Azure::Core::ServiceException and Azure::Core::ProtocolException are two candidates. Azure::Core::OperationFailedException is a poor candidate, because Azure::Core::Operation<T> already exists (sdk/core/azure-core/inc/azure/core/operation.hpp:27) and names a different concept.

The decision adds a dependency between two packages. Event Hubs 1.0.0 now waits for an Azure Core release that contains the new type. Raise this dependency early, because it can move the GA date.

The new type must not repeat the copy defect in #7272. RequestFailedException builds a copy from its Message field instead of from what(), and a copy therefore loses the message.

No public function documents its exceptions

A search for @throw, @throws, and @exception across the installed headers of both packages returns 0 results. Two MUST rules require this documentation: cpp-design-logical-errorhandling-recov-document and cpp-docs-doxygen-failure.

Scope

This issue covers the two Event Hubs packages only. azure-core-amqp contains 345 more throw std:: sites outside its test code and its vendored directory, and one of them is on the Rust receiver attach path (src/impl/rust_amqp/amqp/message_receiver.cpp:173). #7252 records that the Event Hubs public headers embed azure-core-amqp internal types, so the AMQP package reaches the GA contract. #7253 owns that scope decision.

Proposal

• Add a non-HTTP exception type to Azure Core, and take it through the API review. The type carries no HTTP field. [GA blocker]
• Derive EventHubsException from the new type (inc/azure/messaging/eventhubs/eventhubs_exception.hpp:19). This is a breaking change, so it must land before 1.0.0. [GA blocker]
• Classify each of the 24 throw sites as a recoverable error or as a pre-condition violation. Record the classification in this issue. [GA blocker]
• Replace each recoverable error throw with EventHubsException. [GA blocker]
• Replace each pre-condition violation throw with AZURE_ASSERT_MSG. This changes a caller mistake from a catchable exception into an abort, so it is a breaking change. [GA blocker]
• Remove the three throws from the installed public headers as part of the two changes above. [GA blocker]
• Document the exceptions of each public function with a doxygen @throws tag. [GA blocker]
• Coordinate the EventHubsException changes here with the reason enum work in WS6: Finalize the public API before GA #7258. Both changes break callers, and both touch the same class, so one change is cheaper than two. [GA blocker]
• Add a test that makes sure no std::runtime_error escapes a public Event Hubs function. [GA quality bar]

Validation

• A search for throw std:: across the two Event Hubs packages, and outside the test code, returns only sites that the classification approves.
• No installed public header of either package contains a throw.
• Each public function that can throw carries a doxygen @throws tag.
• A caller can catch every recoverable error from the library as an Azure Core exception type.
• The new Azure Core exception type passes the API review, and an Azure Core release contains it.