On Mon, 8 Sep 2025 10:08:05 GMT, Volkan Yazici <vyaz...@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
