On Mon, 24 Nov 2025 11:50:55 GMT, Daniel Fuchs <[email protected]> wrote:
>> Volkan Yazici has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Address review remarks
>
> src/java.net.http/share/classes/jdk/internal/net/http/Http1Response.java line
> 284:
>
>> 282: long clen;
>> 283: try {
>> 284: clen = readContentLength(headers, "", 0);
>
> This does not look right - we should keep -1 as meaning that content length
> was not found.
I've updated it back to `-1` in b491c9bc98a.
> src/java.net.http/share/classes/jdk/internal/net/http/Http1Response.java line
> 289:
>
>> 287: }
>> 288:
>> 289: // Read if there are and less than `MAX_IGNORE` bytes
>
> some missing words here...
Fixed in b491c9bc98a.
> src/java.net.http/share/classes/jdk/internal/net/http/Http1Response.java line
> 291:
>
>> 289: // Read if there are and less than `MAX_IGNORE` bytes
>> 290: if (clen > 0 && clen <= MAX_IGNORE) {
>> 291: return readBody(discarding(), !request.isWebSocket(),
>> executor);
>
> This does not look right, we should not close the connection if we have
> `Content-Length: 0`
I thought we don't need to read if `Content-Length: 0`. Anyway, reverted it in
b491c9bc98a.
> src/java.net.http/share/classes/jdk/internal/net/http/common/Utils.java line
> 1349:
>
>> 1347: // -- toAsciiString-like support to encode path and query URI
>> segments
>> 1348:
>> 1349: public static Long readContentLength(HttpHeaders headers, String
>> errorPrefix, long defaultIfMissing) throws ProtocolException {
>
> I would prefer to use `OptionalLong` rather than nullable `Long`. Or maybe
> it's an oversight and the method is intended to return `long`?
> There seems to be quite some noise generated by trying to avoid using
> OptionalLong and I'm not sure I understand the reason.
> Using Optional/OptionalLong makes it clearer whether the header was found or
> not.
Good catch – a leftover from an earlier refactoring. It can indeed be just
`long` – corrected in b491c9bc98a.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/28431#discussion_r2557667860
PR Review Comment: https://git.openjdk.org/jdk/pull/28431#discussion_r2557668092
PR Review Comment: https://git.openjdk.org/jdk/pull/28431#discussion_r2557669354
PR Review Comment: https://git.openjdk.org/jdk/pull/28431#discussion_r2557671333
