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

[Coleen Phillimore]

On Fri, 10 Jan 2025 01:47:20 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> This bug was introduced by [JDK-8252921](https://bugs.openjdk.org/browse/JDK-8252921) which moved the `unregister_thread_stack_with_NMT` 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!
>> 
>> Update: it was incorrect for the unregister call to ever be in the destructor**, and it only worked by accident. The register/unregister calls have to be done by the current thread on itself. The register call occurs in `Thread::call_run` so logically we should have:
>> 
>> this->register_thread_stack_with_NMT();
>> this->pre_run(); // virtual call for per-thread type init
>> this->run();
>> this->post_run();
>> this->unregister_thread_stack_with_NMT();
>> 
>> However in practice we have to move the unregister inside `post_run` because after `post_run` the thread may have already been deleted and the call would not be valid.
>> 
>> On the JNI side we should simply register in attach, and unregister in detach. When unregister was in the destructor this was all handled implicitly because the destructor was always run by the current thread for itself. When it got moved out of the destructor we not only failed to unregister when detaching, we also failed to unregister if there was a failed attach attempt. The fix here requires that we explicitly unregister on the failure paths, which in turn, to keep things consistent, requires that we explicitly do the `smr_delete` as the final cleanup action.
>> 
>> ** When new Java threads are created the destructor can run in the context of the current creating thread, if starting the new thread fails. If that happens no register call ever occurred and we would then try to unregister in the destructor - which would fail as there was no stack established for the new thread.
>> 
>> We also update the ProblemList and disable -Xcheck:JNI for a test that does actually terminate threads without detaching.
>> 
>> Testing: tiers 1-4
>> 
>> Thanks.
>
> David Holmes has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix order of unregister and smr_delete

This looks good.  It makes sense and keeps things together.  It's sort of unfortunate to repeat the NMT unregistration but smr_delete is also repeated and if there's another thread shutdown action needed outside of exit() we know where to add it (and if there are a lot we could evolve to have some other wrapper for these actions).

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

> 249: 
> 250:   // Logically we should do this->unregister_thread_stack_with_NMT() here, but we
> 251:   // had to move that into post_run() because of the `this` deletion issue.

Yes, this would have made the most sense.  Thanks for the comment.

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

Marked as reviewed by coleenp (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/22924#pullrequestreview-2542461406
PR Review Comment: https://git.openjdk.org/jdk/pull/22924#discussion_r1910389203