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

[Daniel Fuchs]

On Tue, 14 Nov 2023 11:23:57 GMT, Darragh Clarke <dclarke at openjdk.org> wrote:

>> 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`

IIRC the drain() method will be called if the input stream is closed without having read all the bytes, and subsequently  the connection will get closed if the stream is not EOF after drain(); So I believe what Darragh has is actually good enough. It's a bit of belt and braces already.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/16616#discussion_r1392519351