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

[Coleen Phillimore]

On Wed, 8 Jan 2025 21:29:10 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> src/hotspot/share/runtime/javaThread.cpp line 786:
>> 
>>> 784: 
>>> 785: // Shared teardown for all JavaThreads
>>> 786: void JavaThread::post_run() {
>> 
>> I think this would match nonJavaThread::post_run() better if you left this here, and put the NMT call in the detach code.  Ideally detach would call post_run(ExitType) rather than copying exit actions:
>> 
>> https://github.com/coleenp/jdk/blob/master/src/hotspot/share/prims/jni.cpp#L3941
>> 
>> Does this comment make sense?  post_run() seems like it should do all the post run actions or none of them.  Unclear what its purpose is tbh.
>
> 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.  post_run() should just call 'exit' then and 'exit' should call this NMT function and smr_delete().

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

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