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

[Jaikiran Pai]

On Mon, 13 Nov 2023 15:25:25 GMT, Darragh Clarke <dclarke at openjdk.org> wrote:

>> **Problem**
>> `discardRequestBody` calls `InputStream::readAllBytes` to read and discard all the request body bytes. This is somewhat wasteful as they can instead be skipped.
>> 
>> **Changes**
>> - Updated `FileServerHandler::discardRequestBody` to use `InputStream::skip`.
>> - Created skip in `LeftOverInputStream` based on [InputStream](https://github.com/openjdk/jdk/blob/c9657cad124d2be10b8d6006d0ca9a038b1c5945/src/java.base/share/classes/java/io/InputStream.java#L540). This gets used by `FixedLengthInputStream` and `ChunkedInputStream` which previously had been using the skip implementation from `FilteredInputStream` which would have caused blocking.
>> 
>> - Also made a minor change to `LeftOverInputStream::Drain` to change bufSize.
>> 
>> 
>> I ran test tiers 1-3 and all tests are passing with these changes
>
> 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.

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

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