-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8294047: HttpResponseInputStream swallows interrupts #11323
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
8aa21d1
333949d
0d8dd4b
560719e
928176a
83ff9c1
f34f42c
b6a0912
6b9be8a
5f83141
5fc55a3
9937b57
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -481,7 +481,12 @@ private ByteBuffer current() throws IOException { | |
| if (debug.on()) debug.log("Next Buffer"); | ||
| currentBuffer = currentListItr.next(); | ||
| } catch (InterruptedException ex) { | ||
| // continue | ||
| try { | ||
| close(); | ||
| } catch (IOException ignored) { | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't have specific opinion here and this is more of a question - should we be doing: i.e. should we be adding any failure to close() as a suppressed exception to the IOException that we throw?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure whether that would be helpful or confusing.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that's a good point, I don't really have a strong opinion one way or the other on it but would be curious if anyone else does?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's leave it the way you have it now. |
||
| Thread.currentThread().interrupt(); | ||
| throw new IOException(ex); | ||
| } | ||
| } | ||
| assert currentBuffer == LAST_BUFFER || currentBuffer.hasRemaining(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,176 @@ | ||
| /* | ||
| * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved. | ||
| * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. | ||
| * | ||
| * This code is free software; you can redistribute it and/or modify it | ||
| * under the terms of the GNU General Public License version 2 only, as | ||
| * published by the Free Software Foundation. | ||
| * | ||
| * This code is distributed in the hope that it will be useful, but WITHOUT | ||
| * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or | ||
| * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License | ||
| * version 2 for more details (a copy is included in the LICENSE file that | ||
| * accompanied this code). | ||
| * | ||
| * You should have received a copy of the GNU General Public License version | ||
| * 2 along with this work; if not, write to the Free Software Foundation, | ||
| * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. | ||
| * | ||
| * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA | ||
| * or visit www.oracle.com if you need additional information or have any | ||
| * questions. | ||
| */ | ||
|
|
||
| /** | ||
| * @test | ||
| * @bug 8294047 | ||
| * @library /test/lib | ||
| * @run junit HttpResponseInputStreamInterruptTest | ||
| */ | ||
|
|
||
| import com.sun.net.httpserver.HttpExchange; | ||
| import com.sun.net.httpserver.HttpHandler; | ||
| import com.sun.net.httpserver.HttpServer; | ||
| import jdk.test.lib.net.URIBuilder; | ||
| import org.junit.jupiter.api.AfterAll; | ||
| import org.junit.jupiter.api.BeforeAll; | ||
| import org.junit.jupiter.api.Test; | ||
| import org.junit.jupiter.api.TestInstance; | ||
|
|
||
| import java.io.IOException; | ||
| import java.io.InputStream; | ||
| import java.io.OutputStream; | ||
| import java.net.InetAddress; | ||
| import java.net.InetSocketAddress; | ||
| import java.net.URI; | ||
| import java.net.http.HttpClient; | ||
| import java.net.http.HttpRequest; | ||
| import java.net.http.HttpResponse; | ||
| import java.util.concurrent.CountDownLatch; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertThrows; | ||
| import static org.junit.jupiter.api.Assertions.assertTrue; | ||
| import static org.junit.jupiter.api.Assertions.fail; | ||
|
|
||
| @TestInstance(TestInstance.Lifecycle.PER_CLASS) | ||
| public class HttpResponseInputStreamInterruptTest { | ||
|
|
||
| HttpServer server; | ||
| int port; | ||
| private final CountDownLatch interruptReadyLatch = new CountDownLatch(2); | ||
| private final CountDownLatch interruptDoneLatch = new CountDownLatch(1); | ||
| static final String FIRST_MESSAGE = "Should be received"; | ||
| static final String SECOND_MESSAGE = "Shouldn't be received"; | ||
|
|
||
| @BeforeAll | ||
| void before() throws Exception { | ||
| InetAddress loopback = InetAddress.getLoopbackAddress(); | ||
| InetSocketAddress addr = new InetSocketAddress(loopback, 0); | ||
| server = HttpServer.create(addr, 0); | ||
| port = server.getAddress().getPort(); | ||
| Handler handler = new Handler(interruptReadyLatch, interruptDoneLatch); | ||
| server.createContext("/HttpResponseInputStreamInterruptTest/", handler); | ||
| server.start(); | ||
| } | ||
|
|
||
| @AfterAll | ||
| void after() throws Exception { | ||
| server.stop(0); | ||
| } | ||
|
|
||
| @Test | ||
| public void test() throws Exception { | ||
| // create client and interrupter threads | ||
| Thread clientThread = createClientThread(interruptReadyLatch, port); | ||
| Thread interrupterThread = new Thread(() -> { | ||
| try { | ||
| // wait until the clientThread is just about to read the second message sent by the server | ||
| // then interrupt the thread to cause an error to be thrown | ||
| interruptReadyLatch.await(); | ||
| clientThread.interrupt(); | ||
| interruptDoneLatch.countDown(); | ||
| } catch (InterruptedException e) { | ||
| System.out.println("interrupterThread failed"); | ||
| throw new RuntimeException(e); | ||
| } | ||
| }); | ||
|
|
||
| // Start the threads then wait until clientThread completes | ||
| clientThread.start(); | ||
| interrupterThread.start(); | ||
| clientThread.join(); | ||
| } | ||
|
|
||
| static class Handler implements HttpHandler { | ||
|
|
||
| CountDownLatch interruptReadyLatch; | ||
| CountDownLatch interruptDoneLatch; | ||
|
Comment on lines
+107
to
+108
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should be
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe this change was missed and is pending? |
||
|
|
||
| public Handler(CountDownLatch interruptReadyLatch, CountDownLatch interruptDoneLatch) { | ||
| this.interruptReadyLatch = interruptReadyLatch; | ||
| this.interruptDoneLatch = interruptDoneLatch; | ||
| } | ||
|
|
||
| @Override | ||
| public void handle(HttpExchange exchange) throws IOException { | ||
| try (OutputStream os = exchange.getResponseBody()) { | ||
| byte[] workingResponse = FIRST_MESSAGE.getBytes(); | ||
| byte[] errorResponse = SECOND_MESSAGE.getBytes(); | ||
| exchange.sendResponseHeaders(200, workingResponse.length + errorResponse.length); | ||
|
|
||
| // write and flush the first message which is expected to be received successfully | ||
| os.write(workingResponse); | ||
| os.flush(); | ||
|
|
||
| // await the interrupt threads completion, then write the second message | ||
| interruptReadyLatch.countDown(); | ||
| interruptDoneLatch.await(); | ||
| os.write(errorResponse); | ||
| } catch (InterruptedException e) { | ||
| System.out.println("interruptDoneLatch await failed"); | ||
| throw new RuntimeException(e); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| static Thread createClientThread(CountDownLatch interruptReadyLatch, int port) { | ||
| return new Thread(() -> { | ||
| try { | ||
| HttpClient client = HttpClient | ||
| .newBuilder() | ||
| .proxy(HttpClient.Builder.NO_PROXY) | ||
| .build(); | ||
|
|
||
| URI uri = URIBuilder.newBuilder() | ||
| .scheme("http") | ||
| .loopback() | ||
| .port(port) | ||
| .path("/HttpResponseInputStreamInterruptTest/") | ||
| .build(); | ||
|
|
||
| HttpRequest request = HttpRequest | ||
| .newBuilder(uri) | ||
| .GET() | ||
| .build(); | ||
|
|
||
| // Send a httpRequest and assert the first response is received as expected | ||
| HttpResponse<InputStream> response = client.send(request, HttpResponse.BodyHandlers.ofInputStream()); | ||
| String firstOutput = new String(response.body().readNBytes(FIRST_MESSAGE.getBytes().length)); | ||
| assertEquals(firstOutput, FIRST_MESSAGE); | ||
|
|
||
| // countdown on latch, and assert that an IOException is throw due to the interrupt | ||
| // and assert that the cause is a InterruptedException | ||
| interruptReadyLatch.countDown(); | ||
| var thrown = assertThrows(IOException.class, () -> response.body().readAllBytes(), "expected IOException"); | ||
| var cause = thrown.getCause(); | ||
| assertTrue(cause instanceof InterruptedException, cause + " is not an InterruptedException"); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah - and of course the test should verify that the thread interrupted status is set too, since we specified that :-) |
||
| var thread = Thread.currentThread(); | ||
| assertTrue(thread.isInterrupted(), "Thread " + thread + " is not interrupted"); | ||
| } catch (Throwable t) { | ||
| t.printStackTrace(); | ||
| fail(); | ||
| } | ||
| }); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will swallow the interrupt status, you' need to do Thread.currentThread().interrupt() before throwing the IOException.