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

YaSuenag · 2020-12-04T00:47:51Z

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 StreamIteratoras 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();
  }
}

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

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

Reviewers

Daniel Fuchs (@dfuch - Reviewer)
Chris Hegarty (@ChrisHegarty - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1614/head:pull/1614
$ git checkout pull/1614

…OE happens

bridgekeeper · 2020-12-04T00:48:18Z

👋 Welcome back ysuenaga! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2020-12-04T00:48:55Z

@YaSuenag The following label will be automatically applied to this pull request:

net

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

YaSuenag · 2020-12-04T03:13:34Z

Test failure in HotSpot on macOS does not seem to be caused by this change.

mlbridge · 2020-12-04T03:17:00Z

Webrevs

dfuch

Hi Yasumasa,

Thanks for filing the issue and providing a fix.

Before integrating, can you please provide a non regression test that verifies the fix? You can place it in test/jdk/java/net/httpclient/;
Please also make sure that there is no regression in existing tests.

best regards
-- daniel

dfuch · 2020-12-04T09:53:00Z

src/java.net.http/share/classes/jdk/internal/net/http/RequestPublishers.java

+                need2Read = false;
+                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.

You misunderstand. read() should call close(); when read returns -1, hasNext() will take care of updating need2Read and haveNext;

But now I'm no longer convinced that returning -1 when an exception occurs in the wrapped InputStreeam::read call is the right thing to do.

dfuch · 2020-12-04T10:05:42Z

src/java.net.http/share/classes/jdk/internal/net/http/RequestPublishers.java

+                } catch (IOException ex2) {}
                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.

Right. But I am not sure that is the right thing to do. If InputStream::read throws then it's likely that the request body will be missing some bytes, so the request should probably be cancelled/aborted at this point - rather than having a truncated body sent to the server.

YaSuenag · 2020-12-05T04:55:56Z

@dfuch Thanks for your comment! I updated PR.

Close the stream in StreamIterator::hasNext
Throw UncheckedIOException whenever IOE happens
Added testcase for this PR

Could you review again?

ChrisHegarty · 2020-12-10T09:35:05Z

test/jdk/java/net/httpclient/StreamCloseTest.java

+ * @compile ../../../com/sun/net/httpserver/EchoHandler.java
+ * @compile ../../../com/sun/net/httpserver/FileServerHandler.java
+ * @build jdk.test.lib.net.SimpleSSLContext
+ * @build LightWeightHttpServer


It would be preferable to avoid the use of LightWeightHttpServer in this test. The server-side seems trivial enough to implement locally in the test.

I agree with Chris. In addition it will make it possible to check that the server either doesn't receive the request, or get an exception when it tries to read the body bytes.

I updated testcase not to use LightWeightHttpServer.

This test is for HttpClient, and HTTP server might work in undefined behavior if the error happens in client side (I guess it would be handled as an error in most case). So this test does not check request headers/body from the client.

YaSuenag · 2020-12-18T08:07:53Z

@ChrisHegarty @dfuch could you review new change? It updates a testcase.

YaSuenag · 2021-01-07T00:32:42Z

PING: could you review this PR?

dfuch · 2021-01-13T18:59:30Z

Hi Yasumasa,

The new StreamCloseTest seems to be suffering from some race conditions; On windows I saw it failing 29 times out of 50.

config StreamCloseTest.setup(): success
test StreamCloseTest.closeTestOnException(): success
test StreamCloseTest.normallyCloseTest(): failure
java.io.IOException: HTTP/1.1 header parser received no bytes
at java.net.http/jdk.internal.net.http.HttpClientImpl.send(HttpClientImpl.java:584)
at java.net.http/jdk.internal.net.http.HttpClientFacade.send(HttpClientFacade.java:119)
at StreamCloseTest.normallyCloseTest(StreamCloseTest.java:134)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:78)
at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:566)
at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:85)
at org.testng.internal.Invoker.invokeMethod(Invoker.java:639)
at org.testng.internal.Invoker.invokeTestMethod(Invoker.java:821)
at org.testng.internal.Invoker.invokeTestMethods(Invoker.java:1131)
at org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:124)
at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:108)
at org.testng.TestRunner.privateRun(TestRunner.java:773)
at org.testng.TestRunner.run(TestRunner.java:623)
at org.testng.SuiteRunner.runTest(SuiteRunner.java:357)
at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:352)
at org.testng.SuiteRunner.privateRun(SuiteRunner.java:310)
at org.testng.SuiteRunner.run(SuiteRunner.java:259)
at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:52)
at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:86)
at org.testng.TestNG.runSuitesSequentially(TestNG.java:1185)
at org.testng.TestNG.runSuitesLocally(TestNG.java:1110)
at org.testng.TestNG.run(TestNG.java:1018)
at com.sun.javatest.regtest.agent.TestNGRunner.main(TestNGRunner.java:94)
at com.sun.javatest.regtest.agent.TestNGRunner.main(TestNGRunner.java:54)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:78)
at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:566)
at com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127)
at java.base/java.lang.Thread.run(Thread.java:831)
Caused by: java.io.IOException: HTTP/1.1 header parser received no bytes
at java.net.http/jdk.internal.net.http.common.Utils.wrapWithExtraDetail(Utils.java:345)
at java.net.http/jdk.internal.net.http.Http1Response$HeadersReader.onReadError(Http1Response.java:675)
at java.net.http/jdk.internal.net.http.Http1AsyncReceiver.checkForErrors(Http1AsyncReceiver.java:302)
at java.net.http/jdk.internal.net.http.Http1AsyncReceiver.flush(Http1AsyncReceiver.java:268)
at java.net.http/jdk.internal.net.http.common.SequentialScheduler$SynchronizedRestartableTask.run(SequentialScheduler.java:175)
at java.net.http/jdk.internal.net.http.common.SequentialScheduler$CompleteRestartableTask.run(SequentialScheduler.java:147)
at java.net.http/jdk.internal.net.http.common.SequentialScheduler$SchedulableTask.run(SequentialScheduler.java:198)
at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1135)
at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
... 1 more
Caused by: java.io.IOException: An established connection was aborted by the software in your host machine
at java.base/sun.nio.ch.SocketDispatcher.read0(Native Method)
at java.base/sun.nio.ch.SocketDispatcher.read(SocketDispatcher.java:46)
at java.base/sun.nio.ch.IOUtil.readIntoNativeBuffer(IOUtil.java:276)
at java.base/sun.nio.ch.IOUtil.read(IOUtil.java:245)
at java.base/sun.nio.ch.IOUtil.read(IOUtil.java:223)
at java.base/sun.nio.ch.SocketChannelImpl.read(SocketChannelImpl.java:417)
at java.net.http/jdk.internal.net.http.SocketTube.readAvailable(SocketTube.java:1162)
at java.net.http/jdk.internal.net.http.SocketTube$InternalReadPublisher$InternalReadSubscription.read(SocketTube.java:825)
at java.net.http/jdk.internal.net.http.SocketTube$SocketFlowTask.run(SocketTube.java:175)
at java.net.http/jdk.internal.net.http.common.SequentialScheduler$SchedulableTask.run(SequentialScheduler.java:198)
at java.net.http/jdk.internal.net.http.common.SequentialScheduler.runOrSchedule(SequentialScheduler.java:271)
at java.net.http/jdk.internal.net.http.common.SequentialScheduler.runOrSchedule(SequentialScheduler.java:224)
at java.net.http/jdk.internal.net.http.SocketTube$InternalReadPublisher$InternalReadSubscription.signalReadable(SocketTube.java:766)
at java.net.http/jdk.internal.net.http.SocketTube$InternalReadPublisher$ReadEvent.signalEvent(SocketTube.java:949)
at java.net.http/jdk.internal.net.http.SocketTube$SocketFlowEvent.handle(SocketTube.java:245)
at java.net.http/jdk.internal.net.http.HttpClientImpl$SelectorManager.handleEvent(HttpClientImpl.java:976)
at java.net.http/jdk.internal.net.http.HttpClientImpl$SelectorManager.lambda$run$3(HttpClientImpl.java:931)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
at java.net.http/jdk.internal.net.http.HttpClientImpl$SelectorManager.run(HttpClientImpl.java:931)
config StreamCloseTest.teardown(): success

Maybe you should consider using the servers provided by the HttpServerAdapters interface provided in the test directory, as do many other httpclient tests. Look for tests that implement HttpServerAdapters for an example.

YaSuenag · 2021-01-14T02:25:09Z

Thanks @dfuch to notice the error!
I changed to use HttpServerAdapters in StreamCloseTest.java. It works fine on my Linux x64.

Could you review again?

dfuch

The updated test looks good. I ran the httpclient tests ~200 times on all platform and the new test didn't fail. Please integrate and I will sponsor (if you need a sponsor?).

openjdk · 2021-01-14T14:48:51Z

@YaSuenag This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Reviewed-by: dfuchs, chegar

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 516 new commits pushed to the master branch:

be57cf1: 8226416: MonitorUsedDeflationThreshold can cause repeated async deflation requests
c822eda: 8259699: Reduce char[] copying in URLEncoder.encode(String, Charset)
ff3e6e4: 8259773: Incorrect encoding of AVX-512 kmovq instruction
b8ef2ba: 8259563: The CPU model name is printed multiple times when using -Xlog:os+cpu
b040a3d: 8259631: Reapply pattern match instanceof use in HttpClientImpl
3462f7a: 8256955: Move includes of events.hpp out of header files
8b8b1f9: 8259706: C2 compilation fails with assert(vtable_index == Method::invalid_vtable_index) failed: correct sentinel value
ae9187d: 8256109: Create implementation for NSAccessibilityButton protocol
5513f98: 8258010: Debug build failure with clang-10 due to -Wdeprecated-copy
51e14f2: Merge
... and 506 more: https://git.openjdk.java.net/jdk/compare/f83fd4acb4c04285d14eae6b8fee0de58bfcdd45...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

YaSuenag · 2021-01-15T01:57:57Z

The updated test looks good. I ran the httpclient tests ~200 times on all platform and the new test didn't fail. Please integrate and I will sponsor (if you need a sponsor?).

I'm OpenJDK Reviewer (ysuenaga), so I will integrate it.

YaSuenag · 2021-01-15T01:58:09Z

/integrate

openjdk · 2021-01-15T01:59:00Z

@YaSuenag Since your change was applied there have been 527 commits pushed to the master branch:

978bed6: 8259522: Apply java.io.Serial annotations in java.desktop
bf28f92: 8259713: Fix comments about ResetNoHandleMark in deoptimization
4f881ba: 8258652: Assert in JvmtiThreadState::cur_stack_depth() can noticeably slow down debugging single stepping
d18d26e: 8259350: Add some internal debugging APIs to the debug agent
a6b2162: 8259278: Optimize Vector API slice and unslice operations
da6bcf9: 8255019: Shenandoah: Split STW and concurrent mark into separate classes
aba3431: 8258956: Memory Leak in StringCoding on ThreadLocal resultCached StringCoding.Result
8554fe6: 8253866: Security Libs Terminology Refresh
c2a3c7e: 8259727: Remove redundant "target" arguments to methods in Links
1620664: 8259723: Move Table class to formats.html package
... and 517 more: https://git.openjdk.java.net/jdk/compare/f83fd4acb4c04285d14eae6b8fee0de58bfcdd45...master

Your commit was automatically rebased without conflicts.

Pushed as commit e3b548a.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8257736: InputStream from BodyPublishers.ofInputStream() leaks when I…

b7ef4ed

…OE happens

openjdk bot added the net net-dev@openjdk.org label Dec 4, 2020

YaSuenag marked this pull request as ready for review December 4, 2020 03:13

openjdk bot added the rfr Pull request is ready for review label Dec 4, 2020

dfuch suggested changes Dec 4, 2020

View reviewed changes

dfuch reviewed Dec 4, 2020

View reviewed changes

YaSuenag added 2 commits December 5, 2020 13:33

Update patch

7c04e98

Throw UIOE when stream throws IOE

d9628c8

ChrisHegarty reviewed Dec 10, 2020

View reviewed changes

Update testcase

95badeb

Use HttpServerAdapters in StreamCloseTest.java

a081290

dfuch approved these changes Jan 14, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 14, 2021

ChrisHegarty approved these changes Jan 14, 2021

View reviewed changes

openjdk bot closed this Jan 15, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 15, 2021

YaSuenag deleted the JDK-8257736 branch January 16, 2021 13:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

YaSuenag commented Dec 4, 2020 •

edited by openjdk bot

Loading

bridgekeeper bot commented Dec 4, 2020

openjdk bot commented Dec 4, 2020

YaSuenag commented Dec 4, 2020

mlbridge bot commented Dec 4, 2020 •

edited

Loading

dfuch left a comment

dfuch Dec 4, 2020

YaSuenag Dec 4, 2020

dfuch Dec 4, 2020 •

edited

Loading

dfuch Dec 4, 2020

YaSuenag Dec 4, 2020

dfuch Dec 4, 2020

YaSuenag commented Dec 5, 2020

ChrisHegarty Dec 10, 2020

dfuch Dec 10, 2020

YaSuenag Dec 11, 2020

YaSuenag commented Dec 18, 2020

YaSuenag commented Jan 7, 2021

dfuch commented Jan 13, 2021

YaSuenag commented Jan 14, 2021

dfuch left a comment

openjdk bot commented Jan 14, 2021 •

edited

Loading

YaSuenag commented Jan 15, 2021

YaSuenag commented Jan 15, 2021

openjdk bot commented Jan 15, 2021

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

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

Conversation

YaSuenag commented Dec 4, 2020 • edited by openjdk bot Loading

How to reproduce:

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Dec 4, 2020

openjdk bot commented Dec 4, 2020

YaSuenag commented Dec 4, 2020

mlbridge bot commented Dec 4, 2020 • edited Loading

Webrevs

dfuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfuch Dec 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YaSuenag commented Dec 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YaSuenag commented Dec 18, 2020

YaSuenag commented Jan 7, 2021

dfuch commented Jan 13, 2021

YaSuenag commented Jan 14, 2021

dfuch left a comment

Choose a reason for hiding this comment

openjdk bot commented Jan 14, 2021 • edited Loading

YaSuenag commented Jan 15, 2021

YaSuenag commented Jan 15, 2021

openjdk bot commented Jan 15, 2021

YaSuenag commented Dec 4, 2020 •

edited by openjdk bot

Loading

mlbridge bot commented Dec 4, 2020 •

edited

Loading

dfuch Dec 4, 2020 •

edited

Loading

openjdk bot commented Jan 14, 2021 •

edited

Loading