Skip to content
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

8294047: HttpResponseInputStream swallows interrupts #11323

Closed
wants to merge 12 commits into from

Conversation

DarraghClarke
Copy link
Contributor

@DarraghClarke DarraghClarke commented Nov 23, 2022

Currently if a HttpResonseInputStream gets interrupted while reading it will just swallow the exception and continue,

This PR changes it to close the stream and throw an IOException, I added a test to cover this which just uses two threads to read the stream then interrupt it.


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change requires a CSR request to be approved

Issues

  • JDK-8294047: HttpResponseInputStream swallows interrupts
  • JDK-8298163: HttpResponseInputStream swallows interrupts (CSR)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/11323/head:pull/11323
$ git checkout pull/11323

Update a local copy of the PR:
$ git checkout pull/11323
$ git pull https://git.openjdk.org/jdk pull/11323/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11323

View PR using the GUI difftool:
$ git pr show -t 11323

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/11323.diff

@DarraghClarke
Copy link
Contributor Author

/csr needed

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 23, 2022

👋 Welcome back DarraghClarke! 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 openjdk bot added rfr Pull request is ready for review csr Pull request needs approved CSR before integration labels Nov 23, 2022
@openjdk
Copy link

openjdk bot commented Nov 23, 2022

@DarraghClarke has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@DarraghClarke please create a CSR request for issue JDK-8294047 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

@openjdk
Copy link

openjdk bot commented Nov 23, 2022

@DarraghClarke 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.

@openjdk openjdk bot added the net net-dev@openjdk.org label Nov 23, 2022
@mlbridge
Copy link

mlbridge bot commented Nov 23, 2022

@@ -481,7 +481,8 @@ private ByteBuffer current() throws IOException {
if (debug.on()) debug.log("Next Buffer");
currentBuffer = currentListItr.next();
} catch (InterruptedException ex) {
// continue
close();
throw new IOException("interrupted", ex);
Copy link
Member

@dfuch dfuch Nov 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest to throw InterruptedIOException here instead. Also it might be better to swallow any exception close() might throw - using try-finally around close() should do that.

Thread clientThread = createClientThread(countDownLatch, port);
Thread interrupterThread = new Thread(() -> {
try {
countDownLatch.await();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be useful to add a comment before waiting on the latch to explain what you are waiting for.


static class Handler implements HttpHandler {

CountDownLatch countDownLatch;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a better name than countDownLatch could be found - that could give a clue on what this latch is used to wait for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking maybe messageLatch, messageReceivedLatch or clientReadyLatch

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we rename both:

countDownLatch  => interruptReadyLatch
interruptLatch  => interruptDoneLatch

server = HttpServer.create(addr, 0);
port = server.getAddress().getPort();
Handler handler = new Handler(countDownLatch, interruptLatch);
server.createContext("/", handler);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to use a path unique for this test to avoid potential interferences. I would suggest registering the context under "/HttpResponseInputStreamInterruptTest/" instead of "/".

And use that when building the URI below as well.

This is important because the handler is using latches, and therefore can be invoked only once...

Comment on lines 485 to 489
try {
close();
} catch (IOException ignored) {
}
throw new InterruptedIOException();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing that. Maybe we should keep the caught InterruptedException as the cause of the new InterruptedIOException. I'd suggest to add a new utility method to the ...common.Utils class for that. I mean - something like:

    InterruptedIOException Utils.toInterruptedIOException(InterruptedException ex);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So something in utils like return new InterruptedIOException(String.valueOf(exxeption)); ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No something that would call initCause(ex) to set the root cause - as AFAICS there's no constructor that takes a root cause.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah ok, so create a new InterruptedIOException and set the initcause to be the interruptedException.
is there anything else that would need to be set?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

preserving the root cause is the main thing.

Comment on lines +110 to +111
CountDownLatch interruptReadyLatch;
CountDownLatch interruptDoneLatch;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be final

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this change was missed and is pending?

countDownLatch.countDown();
assertThrows(IOException.class, () -> response.body().readAllBytes(), "excepted IOException");
interruptReadyLatch.countDown();
assertThrows(InterruptedIOException.class, () -> response.body().readAllBytes(), "excepted IOException");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe change the message while you're at it since we're actually expecting InterruptedIOException

@@ -486,7 +486,8 @@ private ByteBuffer current() throws IOException {
close();
} catch (IOException ignored) {
}
throw new InterruptedIOException();
// Throw InterruptedIOException where the initCause is set to the caught InterruptedException
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: could you break this long line after "is"

InterruptedIOException interruptedIOException = new InterruptedIOException();
interruptedIOException.initCause(ex);
return interruptedIOException;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The copyright years need updating.

@dfuch
Copy link
Member

dfuch commented Nov 29, 2022

Actually, there's one more thing that we probably should do. The API documentation of HttpResponse.BodySubscribers::ofInputStream has an @apiNote that emphasizes the need to read the stream to exhaustion, or call close, in order to release all resources.
We probably should also add an @apiNote to document that the default implementation returned by that method throw InterruptedIOException if the thread is interrupted while reading from the stream.

}
// Throw InterruptedIOException where the initCause is
// set to the caught InterruptedException
throw Utils.toInterruptedIOException(ex);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you think you need to restores the interrupt status of the thread by invoking the Thread.currentThread().interrupt() method and allowing the caller to detect the interrupt if it needs to ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By convention, any method that exits by throwing an InterruptedException clears interrupt status when it does so. Since we're throwing InterruptedIOException here, I don't think we need to set the interrupt flag. But the caller might do so if they want.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm... Maybe a better solution would be to throw "ClosedByInterruptException" since we're also closing the input stream. And the specification of "ClosedByInterruptException" clearly state that the interrupt status of the thread will be preserved. Good question @vyommani .

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not 100% sure but for me the best option is call close() and just restore the interrupt status let the caller decide how to handle the interrupt.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

InterruptedIOException is not equivalent to InterrupedException in this way AFAIK. The thread interrupt status should be reset if any exception other than InterruptedException is thrown AFAIK, otherwise thread interruption is likely to be lost.

try {
close();
} catch (IOException ignored) {
}
Copy link
Contributor

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.

@DarraghClarke
Copy link
Contributor Author

Thanks for pointing out those issues, with the last few commits I've hopefully addressed everything, but let me know if there is anything else that need to be changed


// countdown on latch, and assert that an IOException is throw due to the interrupt
interruptReadyLatch.countDown();
assertThrows(IOException.class, () -> response.body().readAllBytes(), "expected IOException");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest checking that the exception cause is the expected InterruptedException. If I'm not mistaken something like that should do it:

var thrown = assertThrows(IOException.class, () -> response.body().readAllBytes(), "expected IOException");
var cause = thrown.getCause();
assertTrue(cause instanceof InterruptedException, cause + " is not an InterruptedException");

interruptReadyLatch.countDown();
assertThrows(IOException.class, () -> response.body().readAllBytes(), "expected IOException");
} catch (Exception e) {
throw new RuntimeException(e);
Copy link
Member

@dfuch dfuch Dec 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to catch Throwable, then print the stack trace and call Asserts.fail here - because unhandled exceptions in threads might just be swallowed.
Another possibility is to relay the throwable to the main thread and throw it from the main thread.

throw new RuntimeException(e);
var thrown = assertThrows(IOException.class, () -> response.body().readAllBytes(), "expected IOException");
var cause = thrown.getCause();
assertTrue(cause instanceof InterruptedException, cause + " is not an InterruptedException");
Copy link
Member

Choose a reason for hiding this comment

The 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");

Copy link
Member

@dfuch dfuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - please make sure the test (and tier1 tier2) all pass before integrating

Copy link
Contributor

@vyommani vyommani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks OK to me.

@@ -1178,6 +1178,12 @@ public static BodySubscriber<Path> ofFile(Path file) {
* the underlying HTTP connection to be closed and prevent it
* from being reused for subsequent operations.
*
* @implNote The read method of the default implementation returned
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello Darragh, should we instead reword this as:

@implNote The {@code read} method of the {@code InputStream} returned by the default implementation of this method will throw an {@code IOException} with the {@link Thread#isInterrupted() thread interrupt status set} if the thread is interrupted while blocking on read. In that case, the request will also be cancelled and the {@code InputStream} will be closed.

Additionally, irrespective of whether or not we decide to reword this, this file will need an update for the copyright year.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed but it should be {@linkplain Thread#isInterrupted() thread interrupt status set}

@@ -29,6 +29,7 @@
import java.io.FilePermission;
import java.io.IOException;
import java.io.InputStream;
import java.io.InterruptedIOException;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is now an unused import and we can remove it?

try {
close();
} catch (IOException ignored) {
}
Copy link
Member

Choose a reason for hiding this comment

The 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:

} catch (InterruptedException ex) {
    final IOException toThrow = new IOException(ex);
    try {
        close();
    } catch (IOException ignored) {
        toThrow.addSuppressed(ignored);
    }
    Thread.currentThread().interrupt();
    throw toThrow;    
}

i.e. should we be adding any failure to close() as a suppressed exception to the IOException that we throw?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure whether that would be helpful or confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Member

@jaikiran jaikiran Dec 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's leave it the way you have it now.

}

@Test
public void test() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To keep this method relatively simpler, you could remove the top level try/catch block (which just catches and rethrows) and change this method's signature to throws Exception. If you decide to leave it in the current form, that's fine too.

Copy link
Member

@jaikiran jaikiran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for these changes Darragh. Looks fine to me. There was one review comment by Daniel where he noted that the fields of the Handler in the test should be final (which I agree with). From what I can see, that hasn't been addressed in the updates. In the current test, it doesn't functionally impact anything, so it is OK if you want to leave it in this form.

@openjdk openjdk bot removed the csr Pull request needs approved CSR before integration label Dec 6, 2022
@openjdk
Copy link

openjdk bot commented Dec 6, 2022

@DarraghClarke 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:

8294047: HttpResponseInputStream swallows interrupts

Reviewed-by: dfuchs, vtewari, jpai

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 254 new commits pushed to the master branch:

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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@dfuch, @vyommani, @jaikiran) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Dec 6, 2022
@DarraghClarke
Copy link
Contributor Author

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Dec 7, 2022
@openjdk
Copy link

openjdk bot commented Dec 7, 2022

@DarraghClarke
Your change (at version 9937b57) is now ready to be sponsored by a Committer.

@jaikiran
Copy link
Member

jaikiran commented Dec 8, 2022

/sponsor

@openjdk
Copy link

openjdk bot commented Dec 8, 2022

Going to push as commit 3aa4070.
Since your change was applied there have been 256 commits pushed to the master branch:

  • af8fb7e: 8282578: AIOOBE in javax.sound.sampled.Clip
  • 8b69a2e: 8298099: [JVMCI] decouple libgraal from JVMCI module at runtime
  • 8a9911e: 8295803: Console should be usable in jshell and other environments
  • 5d4c71c: 8281236: (D)TLS key exchange named groups
  • 10356e7: 8298303: (fs) temporarily remove Path.getExtension
  • 3b8c7ef: 8157023: Integrate NMT with JFR
  • e86f31b: 8298301: C2: assert(main_cmp->in(2)->Opcode() == Op_Opaque1) failed: main loop has no opaque node?
  • 8edb98d: 8165943: LineBreakMeasurer does not measure correctly if TextAttribute.TRACKING is set.
  • 3934484: 8298205: Prefer Member Initialization Lists for JFR classes in os_perf.hpp
  • 389b8f4: 8297298: SequenceInputStream should override transferTo
  • ... and 246 more: https://git.openjdk.org/jdk/compare/f4b5065c37e86f4b2ca26da6ce678febe4a52950...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Dec 8, 2022
@openjdk openjdk bot closed this Dec 8, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Dec 8, 2022
@openjdk
Copy link

openjdk bot commented Dec 8, 2022

@jaikiran @DarraghClarke Pushed as commit 3aa4070.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated net net-dev@openjdk.org
6 participants