Conversation
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
| - **Hooks called once per execution** (`readBeforeExecution`, | ||
| `readAfterExecution`): Errors are collected across all interceptors before any | ||
| further action is taken. If multiple interceptors raise errors, the last error | ||
| wins and earlier ones are logged and dropped. |
There was a problem hiding this comment.
Open discussion: In my opinion, the SRA does not offer sufficient guidance on error handling within interceptor hooks. None of the prose mentions errors and only the interface JavaDocs offer any hints about how to handle errors in specific hooks. The pattern you've identified here ("hooks called once per execution") doesn't precisely match those JavaDocs:
| Hook | Mutability | Execution count | SRA error behavior | Draft SCG error behavior |
|---|---|---|---|---|
readBeforeExecution |
Immutable | 1 per call | Accumulate | Accumulate |
modifyBeforeSerialization |
Mutable | 1 per call | Throw first | Accumulate |
readBeforeSerialization |
Immutable | 1 per call | Throw first | Accumulate |
readAfterSerialization |
Immutable | 1 per call | Throw first | Accumulate |
modifyBeforeRetryLoop |
Mutable | 1 per call | Throw first | Accumulate |
readBeforeAttempt |
Immutable | 1 per attempt | Accumulate | Throw first |
modifyBeforeSigning |
Mutable | 1 per attempt | Throw first | Throw first |
readBeforeSigning |
Immutable | 1 per attempt | Throw first | Throw first |
readAfterSigning |
Immutable | 1 per attempt | Throw first | Throw first |
modifyBeforeTransmit |
Mutable | 1 per attempt | Throw first | Throw first |
readBeforeTransmit |
Immutable | 1 per attempt | Throw first | Throw first |
readAfterTransmit |
Immutable | 1 per attempt | Throw first | Throw first |
modifyBeforeDeserialization |
Mutable | 1 per attempt | Throw first | Throw first |
readBeforeDeserialization |
Immutable | 1 per attempt | Throw first | Throw first |
readAfterDeserialization |
Immutable | 1 per attempt | Throw first | Throw first |
modifyBeforeAttemptCompletion |
Mutable | 1 per attempt | Throw first | Throw first |
readAfterAttempt |
Immutable | 1 per attempt | Accumulate | Throw first |
modifyBeforeCompletion |
Mutable | 1 per call | Throw first | Accumulate |
readAfterExecution |
Immutable | 1 per call | Accumulate | Accumulate |
I'd like to propose a more straightforward rule: immutable hooks accumulate errors, mutable hooks do not. It doesn't matter whether the hook is per call or per attempt, it doesn't matter where/when in the pipeline it happens, it's just down to mutability.
What do we think about adopting that stance?
There was a problem hiding this comment.
My translation here definitely wasn't correct, and I'll fix that. The intent of the SRA accumulating hooks was to guarantee that each interceptor is called because those hooks in particular represent key lifecycle checkpoints where resources are likely to be created or destroyed.
I think that accumulating on all the immutable hooks is probably fine. It can let interceptors know how far a request got if they care to track that. And since they can't modify anything anyway, it should be fine.
| ### Hook input types | ||
|
|
||
| Each hook receives a typed input object that contains only the data available at | ||
| that stage of the pipeline, along with a [context object](#typed-context). Using | ||
| typed inputs prevents interceptors from accidentally accessing data that doesn't | ||
| exist yet (for example, trying to read the transport response before a request | ||
| has been sent). |
There was a problem hiding this comment.
Correctness: These interfaces don't comprehend error tracking, only success tracking. The SRA used a fictitious Result<SuccessType, ErrorType> to handle this.
There was a problem hiding this comment.
Rust-style result types aren't idiomatic in Java. I can add some more details about that option though.
There was a problem hiding this comment.
Result is one possible way of handling but not the only one...we could add @Nullable Throwable members to the various hook interfaces. Whatever interface we show must be able to handle prior successful responses and failed ones.
There was a problem hiding this comment.
Nit: This draft omits SRA's guidance on the type of code that should run in hooks:
Ensure that customers do not perform blocking operations in interceptor hooks. Where this is impossible, they are documented that the hooks must not perform blocking operations.
I think we should carry this forward with some light rewording.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.