On Fri, 4 Dec 2020 09:53:00 GMT, Daniel Fuchs <dfu...@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
