On Mon, 8 Sep 2025 13:32:26 GMT, Daniel Fuchs <[email protected]> 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
