RFR: 8367067: Improve exception handling in HttpRequest.BodyPublishers [v2]

[Volkan Yazici]

On Mon, 8 Sep 2025 13:32:26 GMT, Daniel Fuchs <dfuchs at openjdk.org> wrote:

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

Thanks for the feedback. I’ll set this exploration aside until after the PR. If I get convincing results, I’ll knock on your door again.

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

It is provided in the ctor above using `Utils.BUFSIZE`, which reads the `jdk.httpclient.bufsize`  system property, and that can be negative.

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

> Then why can't we simply `throw e` instead?

Rethrows generally start hurting while debugging, hence I favor an accurate stack trace instead – granted no performance implications.

> did you observe that the additional stack trace information added by the wrapping in UncheckedIOException was useful

Not really. Switched to rethrows in e9bc9e769c3.

>> 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)

Switched to rethrows in e9bc9e769c3.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/26876#discussion_r2330943110
PR Review Comment: https://git.openjdk.org/jdk/pull/26876#discussion_r2330967491
PR Review Comment: https://git.openjdk.org/jdk/pull/26876#discussion_r2331034601
PR Review Comment: https://git.openjdk.org/jdk/pull/26876#discussion_r2331035119