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

[Johan Sjölen]

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

Thank you again, this LGTM.

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

Marked as reviewed by jsjolen (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/22924#pullrequestreview-2549447128