RFR (S) 8235962: os::current_thread_id() is not signal safe on macOS

[David Holmes]

Hi Gerard,

Thanks for tackling this. All extremely confusing as usual :) far too 
many different notions of "thread identity".

On 14/01/2020 4:43 am, gerard ziemski wrote:
> hi all,
> 
> Before JDK-8022808 we used to call "mach_thread_self()" to obtain a 
> thread id, but back then we forgot to release the returned mach port, 
> which would make a long running java process (like Kitchensink) 
> eventually run out of resources and hang it.
> 
> To solve that, in JDK-8022808, we switched to using 
> "pthread_mach_thread_np(pthread_self())", which returns a static mach 
> port, which does not need to be released.
> However, the "pthread_mach_thread_np()" API, which we started using is 
> specifically knownn as not async-signal-safe API and that might cause an 
> issue, because we use it to generate thread ids in hs_err_pid.log crash 
> log files in our signal handler, which requires that only 
> async-signal-safe API be used.
> 
> Our crash handler is best effort approach, and to this extend I'd like 
> to propose a fix, which reverts back to using "mach_thread_self()", 
> which I must admit is unknown whether it itself is async-signal-safe, 
> though it's not part of pthread API, like "pthread_mach_thread_np()" 
> which itself is not part of async-signal-safe APIs (IEEE Std 1003.1, 
> 2016). Additionally, the reporter of the issue suggest that it solves 
> the actual issue with using "pthread_mach_thread_np()" for them in their 
> project, so let's benefit from their experience.

Okay that seems reasonable.

> In this fix we revert back to using "mach_thread_self()" API, but make 
> sure to release the mach port, and use MACH_PORT_VALID() macro to verify 
> the validity of mach port.

I'm unclear on this aspect. Now that we are releasing it doesn't that 
mean that the port value could be reused and applied to a different 
thread? It isn't at all clear to me what a "mach port" actually means 
and what releasing it (or failing to) means.

> bug: https://bugs.openjdk.java.net/browse/JDK-8235962
> webrev: http://cr.openjdk.java.net/~gziemski/8235962_rev1/index.html

I found the refactoring around _unique_thread_id a bit hard to follow. 
Your new set_unique_thread_id needs to use "#ifdef __APPLE__" otherwise 
it won't compile on other BSD systems.

Thanks,
David
-----

> Tested with Mach5 tier1,2,3,4,5 and Kitchensink24 and 
> Kitchensink24Stress locally on Mac laptop over 2 days.
> 
> 
> cheers