RFR: 8266310: deadlock while loading the JNI code [v5]

[Aleksei Voitylov]

On 25/05/2021 04:44, David Holmes wrote:
> On 25/05/2021 7:53 am, Aleksei Voitylov wrote:
>> On Mon, 24 May 2021 06:24:15 GMT, David Holmes <dholmes at openjdk.org>
>> wrote:
>>
>>>> Aleksei Voitylov has updated the pull request incrementally with
>>>> one additional commit since the last revision:
>>>>
>>>>    fix whitespace
>>>
>>> src/java.base/share/classes/jdk/internal/loader/NativeLibraries.java
>>> line 511:
>>>
>>>> 509:             if (currentLock.getCounter() == 1) {
>>>> 510:                 // unlock and release the object if no other
>>>> threads are queued
>>>> 511:                 currentLock.unlock();
>>>
>>> This isn't atomic so I don't see how it can work as desired. Overall
>>> I don't understand what the semantics of this "counted lock" are
>>> intended to be.
>>
>> Hi David,
>>
>> The locking strategy is as follows:
>>
>> CountedLock is a subclass of ReentrantLock that allows exact counting
>> of threads that intend to acquire the lock object. Each time a thread
>> calls acquireNativeLibraryLock() with a certain name, either a new
>> CountedLock object is allocated and assigned 1 as the counter, or an
>> existing CountedLock is incremented prior to invocation of the lock()
>> method on it. The increment operation to the lock object is performed
>> in the context of execution of compute() method, which is executed
>> atomically. This allows to correctly dispose the CountedLock object
>> when the last thread in the queue leaves releaseNativeLibraryLock().
>> The entire remapping function passed to computeIfPresent() method is
>> executed atomically.
>
> So the counting is trying to be an in-use count so that it can be
> deleted when not needed? 
Yes. The ReentrantLock does not provide an exact count of threads in the
queue, so we had to invent something here. ReentrantLock only has the
lock.state that's exact which is the number of holds by the same thread
which is not what we need to make sure the object can be disposed.
> I'm still not clear on exactly what this counting is doing (partly
> because I have trouble reading the lambda expressions that relate to
> the lock).
The counter is incremented each time a thread calls
acquireNativeLibraryLock(), regardless of that it is the same or a
different thread.
>
>> Could you be more specific on what is not performed atomically?
>
> The code I referenced. The counter is not protected by the lock that
> it counts. 
Yes, but it is protected by a barrier created by synchronized() block in
the computeIfPresent() code. The entire lambda is executed inside that
synchronized() block. The ReentrantLock.unlock() is called here so that
the calling thread releases the per-library lock object, prior to
"forgetting" the reference to that object.
> In the code above we hold the lock and check if the counter == 1
> before unlocking. But the counter is incremented without the lock
> held, so as soon as we have called getCounter() the count could have
> changed.
The counter is incremented and decremented in the lambda expression
executed inside computeXXX() method which blocks on the hash map node
containing the lock object. That lambda is guaranteed to be executed
atomically for all threads referencing the same key.
>
> David
> -----
>
>> -------------
>>
>> PR: https://git.openjdk.java.net/jdk/pull/3976
>>