Bug ID: JDK-8286088 add comment to InstallAsyncExceptionHandshake destructor

Versions (Unresolved/Resolved/Fixed)

The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.

JDK 19
19 masterFixed

[~dholmes] requested a comment be added after the fix for:

    JDK-8284632 runtime/Thread/StopAtExit.java possibly leaking memory again

was integrated.

src/hotspot/share/runtime/thread.cpp:

1673:  ~InstallAsyncExceptionHandshake() {
1674:    delete _aeh;

Member
@dholmes-ora dholmes-ora 20 hours ago

A comment would have been good. I'm assuming:

// Can only be non-null if this handshake was never actually executed.
Member Author
@dcubed-ojdk dcubed-ojdk 20 hours ago

Since we clear _aeh on L1679 below when do_thread() is executed, _aeh can
only be non-NULL when ~InstallAsyncExceptionHandshake() runs if do_thread()
was never executed.

I can add that comment above L1674 in a separate bug fix if you wish.
@robehn - do you concur that a comment here would be good?
Member
@dholmes-ora dholmes-ora 17 hours ago

It was the fact we null it and delete it that initially caused a "What the???" reaction. :) So I think I comment would be good but can wait.
Member
@robehn robehn 10 hours ago

Yes add a comment.

Changeset: efcd3d3a Author: Daniel D. Daugherty <dcubed@openjdk.org> Date: 2022-05-03 21:26:53 +0000 URL: https://git.openjdk.java.net/jdk/commit/efcd3d3a8fd19909ef21a07cc61613aad4dbe145
03-05-2022
Discussed the comment with [~dholmes] via Slack and simplified it: $ git diff -r master diff --git a/src/hotspot/share/runtime/thread.cpp b/src/hotspot/share/runtime/thread.cpp index 006df6d1f7f..b61d1243fe1 100644 --- a/src/hotspot/share/runtime/thread.cpp +++ b/src/hotspot/share/runtime/thread.cpp @@ -1671,6 +1671,7 @@ public: InstallAsyncExceptionHandshake(AsyncExceptionHandshake* aeh) : HandshakeClosure("InstallAsyncException"), _aeh(aeh) {} ~InstallAsyncExceptionHandshake() { + // If InstallAsyncExceptionHandshake was never executed we need to clean up _aeh. delete _aeh; } void do_thread(Thread* thr) {
03-05-2022
A pull request was submitted for review. URL: https://git.openjdk.java.net/jdk/pull/8522 Date: 2022-05-03 19:46:34 +0000
03-05-2022
Here's the context diff for the proposed comment: $ git diff diff --git a/src/hotspot/share/runtime/thread.cpp b/src/hotspot/share/runtime/thread.cpp index 006df6d1f7f..27d848dff28 100644 --- a/src/hotspot/share/runtime/thread.cpp +++ b/src/hotspot/share/runtime/thread.cpp @@ -1671,6 +1671,9 @@ public: InstallAsyncExceptionHandshake(AsyncExceptionHandshake* aeh) : HandshakeClosure("InstallAsyncException"), _aeh(aeh) {} ~InstallAsyncExceptionHandshake() { + // Usually InstallAsyncExceptionHandshake is executed by do_thread() below + // and _aeh is cleared. If InstallAsyncExceptionHandshake is not executed, + // then the non-null _aeh has to be deleted from here. delete _aeh; } void do_thread(Thread* thr) {
03-05-2022