Azure / Azure/azure-sdk-for-cpp

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

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

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` 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 #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.

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.