RFR: 8257736: InputStream from BodyPublishers.ofInputStream() leaks when IOE happens

[Yasumasa Suenaga]

On Fri, 4 Dec 2020 09:53:00 GMT, Daniel Fuchs <dfuchs at openjdk.org> wrote:

>> `InputStream` from `BodyPublishers.ofInputStream()` is usually closed when the stream reaches EOF. However IOE handler returns without closing.
>> 
>> I confirmed this problem in `BodyPublishers.ofInputStream()`, but I think `BodyPublishers.ofFile()`has same problem because it also use `StreamIterator`as well as `ofInputStream()`.
>> 
>> 
>> # How to reproduce:
>> 
>> Following code (Test.java) attempts to post body from `TestInputStream` which throws IOE in `read()`. "close called" is shown on the console if `close()` is called.
>> 
>> import java.io.*;
>> import java.net.*;
>> import java.net.http.*;
>> 
>> public class Test{
>> 
>>   private static class TestInputStream extends InputStream{
>> 
>>     public TestInputStream(){
>>       super();
>>       System.out.println("test c'tor");
>>     }
>> 
>>     @Override
>>     public int read() throws IOException{
>>       System.out.println("read called");
>>       throw new IOException("test");
>>     }
>> 
>>     @Override
>>     public void close() throws IOException{
>>       System.out.println("close called");
>>       super.close();
>>     }
>> 
>>   }
>> 
>>   public static void main(String[] args) throws Exception{
>>     var http = HttpClient.newHttpClient();
>>     var request = HttpRequest.newBuilder()
>>                              .uri(URI.create("http://httpbin.org/post"))
>>                              .POST(HttpRequest.BodyPublishers.ofInputStream(() -> new TestInputStream()))
>>                              .build();
>>     http.send(request, HttpResponse.BodyHandlers.discarding());
>>     System.out.println("Press any key to exit...");
>>     System.in.read();
>>   }
>> }
>
> src/java.net.http/share/classes/jdk/internal/net/http/RequestPublishers.java line 422:
> 
>> 420:             } catch (IOException ex) {
>> 421:                 need2Read = false;
>> 422:                 haveNext = false;
> 
> This shouldn't be required - `need2Read`/`haveNext` will be set by `hasNext()`; I'd prefer if we kept the logic there.

We can close the stream in `hasNext()`, but we need to move `close()` from `read()`.
`need2Read` and `haveNext` will be set to `false` if `read()` returns -1. However `read()` returns -1 when IOE or EOF happen. Is it ok? I concern to change location of `close()` - it means the change of side-effect.

> src/java.net.http/share/classes/jdk/internal/net/http/RequestPublishers.java line 426:
> 
>> 424:                     is.close();
>> 425:                 } catch (IOException ex2) {}
>> 426:                 return -1;
> 
> } catch (IOException ex2) {}
>                 return -1;
> 
> I wonder if the first exception `ex` should actually be rethrown here instead of returning `-1`. Have you tried to explore this possibility?

`read()` is not have IOE as checked exception, and also currently IOE is ignored.
So I ignored IOE at `close()` in this PR to minimize side-effect.

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

PR: https://git.openjdk.java.net/jdk/pull/1614