RFR: 8367067: Improve exception handling in HttpRequest.BodyPublishers

[Daniel Fuchs]

On Mon, 8 Sep 2025 10:08:05 GMT, Volkan Yazici <vyazici at openjdk.org> wrote:

>> Improves exception handling of built-in `HttpClient.BodyPublisher`s to ensure exceptions get propagated in a way compliant with the reactive specification.
>
> src/java.net.http/share/classes/jdk/internal/net/http/PullPublisher.java line 39:
> 
>> 37: class PullPublisher<T> implements Flow.Publisher<T> {
>> 38: 
>> 39:     // Only one of `iterable` or `throwable` should be null, and the other non-null. throwable is
> 
> `PullPublisher` logic is branching on the `throwable == null` condition on several places – this increases code size and complexity. Plus, all `PullPublisher::new` call sites know at compile time whether `throwable` is null or not. I think `PullPublisher` should only accept a `CheckedIterable`, and we should create a new, say, `ImmediatelyFailingPublisher` to cover the cases where a `Throwable` needs to be passed to subscribers verbatim. I did not carry out this improvement in this PR to avoid scope creep, but I'm inclined to create a ticket for it. WDYT?

You might discover that it's not as simple, as the case where the throwable is known in advance folds within the case where getting the interator throws...
You'd still need to be able to create a failed subscription at line 73 below.

I am not opposed to exploring that route though - and see if it really simplifies the code.

> src/java.net.http/share/classes/jdk/internal/net/http/RequestPublishers.java line 78:
> 
>> 76:         private ByteArrayPublisher(byte[] content, int offset, int length, int bufSize) {
>> 77:             Objects.checkFromIndexSize(offset, length, content.length);     // Implicit null check on `content`
>> 78:             if (bufSize <= 0) {
> 
> New validation to checking on `bufSize`.

Can bufSize be <= 0 in our implementation? Where does bufSize comes from? If it can't be <= 0 in our implemlementation then an `assert` would be more appropriate.

> src/java.net.http/share/classes/jdk/internal/net/http/RequestPublishers.java line 357:
> 
>> 355:                     haveNext = false;
>> 356:                     need2Read = false;
>> 357:                     throw new IOException(e);
> 
> We don't convert `IOE`s to `UncheckedIOE`s anymore. This effectively makes the `HttpClientImpl::send` exception translation logic branching on `if (throwable instanceof IOException)` to kick in.

That's a good point. The question is whether the call to read() that throws will be in the exception stack trace or not. We're in an asynchronous system so it might not. @vy were you able to trick the code into generating an IOException here - and did you observe that the additional stack trace information added by the wrapping in UncheckedIOException was useful? Because if it's not, we can simply rethrow e as @liach suggests.

> src/java.net.http/share/classes/jdk/internal/net/http/RequestPublishers.java line 486:
> 
>> 484:             } catch (IOException ioe) {
>> 485:                 terminated = true;
>> 486:                 throw new IOException(ioe);
> 
> We don't convert `IOE`s to `UncheckedIOE`s anymore. This effectively makes the `HttpClientImpl::send` exception translation logic branching on `if (throwable instanceof IOException)` to kick in.

(same question that @liach asked before)

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/26876#discussion_r2330265962
PR Review Comment: https://git.openjdk.org/jdk/pull/26876#discussion_r2330275013
PR Review Comment: https://git.openjdk.org/jdk/pull/26876#discussion_r2330291788
PR Review Comment: https://git.openjdk.org/jdk/pull/26876#discussion_r2330313850