On Wed, 10 Dec 2025 19:54:58 GMT, Daniel Fuchs <[email protected]> wrote:
>> Doh! Very good catch. I've fixed this in 024e355f034. I've copied the
>> `errorHandler` we pass to `ResponseSubscribers::getBodyAsync`. Note that
>> I've slightly changed the `errorHandler` to have a `finally` block:
>>
>>
>> try {
>> subscriber.onError(error);
>> cf.completeExceptionally(error);
>> } finally {
>> asyncReceiver.setRetryOnError(false);
>> asyncReceiver.onReadError(error);
>> }
>>
>>
>> This is what the `executor.execute(() -> ...)` block does too, which I
>> presume to guard against misbehaving `subscriber::onError` and
>> `cf::completeExceptionally`.
>>
>> @dfuch, I'm not resolving this conversation yet. Would you mind reviewing
>> 024e355f034 and letting me know if it is okay, please?
>
> Looks OK - out of curiosity, is the try-finally simple hygienic or did you
> observe a case where it was required?
@djelinski also DM'ed me about the same questions yesterday. I unfortunately
don't have a very satisfactory answer with complete certainty. In short, I've
copied the existing convention in the area that I touched.
1. This convention – having `subscriber.onError(error)` and
`cf.completeExceptionally(error)` in `try`, and
`asyncReceiver.setRetryOnError(false)` and `asyncReceiver.onReadError(error)`
in `finally` – was already present in the code before my changes.
2. Even though the handler passed to `ResponseSubscribers.getBodyAsync` did not
have a `try`/`finally`, I did not opt for no-`try`/`finally` in
`errorNotifier`, because AFAICT, `ResponseSubscribers.getBodyAsync` appears
like an oversight, and all earlier lines always had
`asyncReceiver.onReadError(error)` and `subscriber.onError(error)` in `finally`.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/28431#discussion_r2610351303
