On Tue, 14 Nov 2023 08:08:06 GMT, Jaikiran Pai <[email protected]> wrote:
>> Darragh Clarke has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> updated drain to use the new skip method
>
> src/jdk.httpserver/share/classes/sun/net/httpserver/simpleserver/FileServerHandler.java
> line 162:
>
>> 160: private static void discardRequestBody(HttpExchange exchange)
>> throws IOException {
>> 161: try (InputStream is = exchange.getRequestBody()) {
>> 162: is.skip(Integer.MAX_VALUE);
>
> Hello Darragh, since the goal is to read all bytes of the InputStream and
> then close it, I wonder if we should instead do:
>
>
> try (InputStream is = exchange.getRequestBody()) {
> boolean eof = false;
> while (!eof) {
> is.skip(Long.MAX_VALUE);
> // check if stream has reached EOF
> final int nextByte = is.read();
> if (nextByte < 0) {
> eof = true;
> }
> }
>
> }
>
> (We could have just called `while ( !eof) { eof = is.drain(Long.MAX_VALUE);
> }`, but drain() is a method on an internal implementation of the
> `InputStream`.)
>
> In its current form (even before the changes proposed in the PR), the use of
> `readAllBytes()` doesn't guarantee that the entire stream is read. That can
> then mean that we might close the (socket) input stream before reading the
> entire body.
Hey Jaikiran thanks for the suggestion, I think that's a good point.
I had set it to `is.skip(Integer.MAX_VALUE)` because `readAllBytes` also used
`Integer.MAX_VALUE`, but as you point out that might not guarantee that all
bytes get read.
I was wondering if @dfuch might know if theres any risk or issues related to
large requestBodys that are ≥ `Long.MAX_VALUE`
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16616#discussion_r1392436859
