RFR: 8290043: serviceability/attach/ConcAttachTest.java failed "guarantee(!CheckJNICalls) failed: Attached JNI thread exited without being detached"

[David Holmes]

On Wed, 8 Jan 2025 22:49:39 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> Thanks for looking at this @coleenp 
>> 
>> `post_run` does perform all "post run actions" but that doesn't mean it has to list them individually rather than being bundled in other functions. `JavaThread::exit` encapsulates "all" the things that have to be done as a thread exits - so moving the NMT teardown to there makes sense to me. If anything what is missing is an equivalent `NonJavaThread::exit`.
>> 
>> Detaching and terminating have to perform similar actions to a point but I would not want to conflate things by having a detaching thread call post_run as it never called pre_run or run! `JavaThread::exit` captures the shared "termination" logic for the two cases.
>
> This is very asymmetrical to me.  Maybe JavaThread::post_run() should just call 'exit' then and 'exit' should call this NMT function and smr_delete().

`smr_delete` is deliberately not put into `exit` so that it is guaranteed noone ever mistakenly puts new exit logic after the delete occurs.

`JavaThread::exit` is the primary location for thread "termination" actions, so it makes sense to have the NMT function there.

There is no `NonJavaThread::exit()` so all exit actions, including the NMT one, go into `post_run()`.

Any assymmetry is with the fact there is no `NonJavaThread::exit`, but that is not the issue this PR is trying to fix.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/22924#discussion_r1908135917