RFR: 8319531: FileServerHandler::discardRequestBody could be improved [v3]

[Darragh Clarke]

On Tue, 14 Nov 2023 08:08:06 GMT, Jaikiran Pai <jpai at openjdk.org> 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