On Thu, 1 Jun 2023 11:09:37 GMT, Daniel Fuchs <[email protected]> wrote:
>> 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 `ConnectionAborter` class 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 generic `IOException: connection closed locally` is
>> raised at the `SocketTube` level, 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.
>
> Daniel Fuchs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Review feedback
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.
src/java.net.http/share/classes/jdk/internal/net/http/Exchange.java line 176:
> 174: } else {
> 175: this.connection = null;
> 176: this.cause = null;
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;
}
}
-------------
Marked as reviewed by jpai (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/14251#pullrequestreview-1456894820
PR Review Comment: https://git.openjdk.org/jdk/pull/14251#discussion_r1214079485
