8309200: java/net/httpclient/ExecutorShutdown fails intermittently, if connection closed during upgrade#14251
Conversation
|
👋 Welcome back dfuchs! A progress list of the required criteria for merging this PR into |
openjdk
bot
changed the title
Webrevs
|
| HttpConnection connection; | ||
| Throwable cause; | ||
| synchronized (this) { | ||
| if ((cause = this.cause) == null) { |
There was a problem hiding this comment.
let's extract the assignment to a separate statement. Code is a bit hard to read.
| closeRequested = true; | ||
| } else { | ||
| this.connection = null; | ||
| this.cause = null; |
There was a problem hiding this comment.
Hello Daniel, I had to read this a few times to understand why we are resetting this.cause, which we potentially set a few lines above, to null here. From what I understand, we only want to set this.cause when the connection is null. Do you think this code could instead of written as:
Throwable cause;
synchronized (this) {
cause = this.cause;
if (cause == null) {
cause = error;
}
connection = this.connection;
if (connection == null) {
closeRequested = true;
this.cause = cause;
} else {
this.connection = null;
this.cause = null;
}
}There was a problem hiding this comment.
Much better yes. I'll steal it!
jaikiran
left a comment
jaikiran
left a comment
There was a problem hiding this comment.
The changes look fine to me. I have a minor comment about the code in the ConnectionAborter, which I've added inline. It's OK with me if you prefer to maintain it in its current form in the PR.
|
@dfuch 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: 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 45 new commits pushed to the
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 |
dfuch
changed the title
jaikiran
left a comment
jaikiran
left a comment
There was a problem hiding this comment.
Thank you Daniel for the update. Looks good to me.
|
/integrate |
|
Going to push as commit 931913f.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
4 participants
The ExecutorShutdown test has been observed failing intermittently, notably if by misfortune the shutdown sequence causes a connection to get aborted while upgrading. The issue is that the
ConnectionAborterclass that allows to mark the connection as being scheduled for closing before a handle to the connection is actually available isn't forwarding the original exception for which closing the connection was requested. When the connection is eventually closed, a genericIOException: connection closed locallyis raised at theSocketTubelevel, which unfortunately can race with the original cause.The fix makes it possible to relay the original cause to the place where the IOException is raised, in order to set it as the cause of the new exception.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/14251/head:pull/14251$ git checkout pull/14251Update a local copy of the PR:
$ git checkout pull/14251$ git pull https://git.openjdk.org/jdk.git pull/14251/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 14251View PR using the GUI difftool:
$ git pr show -t 14251Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/14251.diff
Webrev
Link to Webrev Comment