Staffan, the change looks good. (Not a Reviewer).
One thing to point out though, if we would leak ports the following line would get stuck in an infinite loop (pthread_mach_thread_np calls _pthread_lookup_thread which calls _pthread_find_thread which will spin forever if the ports part of the pthread struct is zero - which it will be if we are out of ports). mach_port_t thread_id = ::pthread_mach_thread_np(::pthread_self()); That makes the guarantee line redundant since it will never be reached. Maybe it would be good if we had a better way of checking if we are out of resources. /R On Aug 19, 2013, at 11:08 AM, Staffan Larsen wrote: > We are using the mach thread port name as the os thread identifier on OS X. > We get this value by calling mach_thread_self(), but we (I) didn't realize > that this "port" resource is reference counted and mach_thread_self() > increases the reference count, but we never call mach_port_deallocate() to > decrease the reference count. This leads to a resource leak and eventually > mach_thread_self() will return 0 to indicate that we are out of ports. > Running out of ports causes the pthreads implementation (which is built on > top of mach) to spin forever in some calls (most notably in this case > pthread_kill()). > > One way to fix this is to make sure we call mach_port_deallocate() for each > call to mach_thread_self(). However, this adds extra complexity to the code > and also makes it slower. Another solution is to ask pthreads for the mach > port name that it already has. Pthreads provides the function > pthread_mach_thread_np() for this purpose. In this case there is no need to > deallocate the port since pthread_mach_thread_np() will not increase the > reference count (and pthreads will call deallocate on the port once the > thread terminates). > > This fix has been verified by seeing that the number of ports the process has > allocated does not increase in Activity Monitor (or top). It has also passed > JPRT testing on all platforms. > > webrev: http://cr.openjdk.java.net/~sla/8022808/webrev.00/ > (there is not publicly visible bug for this) > > Thanks, > /Staffan
