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

[David Holmes]

On Wed, 8 Jan 2025 21:06:17 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> This bug was introduced by [JDK-8252921](https://bugs.openjdk.org/browse/JDK-8252921) which moved the unregister call from the thread destructor (because it was not called for all threads) to the post_run() method. But this totally overlooked native threads that attach and detach! This is easily fixed by moving the unregister call to the end of JavaThread::exit.
>> 
>> We also update the ProblemList and disable -Xcheck:JNI for a test that does actually terminate threads without detaching.
>> 
>> Testing: tiers 1-4
>> 
>> Thanks.
>
> 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!

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

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