Unsafe: removing the monitorEnter/monitorExit/tryMonitorEnter methods

[Peter Levart]

On 02/27/2014 10:58 AM, David Holmes wrote:
> On 27/02/2014 6:38 PM, Peter Levart wrote:
>> On 02/27/2014 08:41 AM, David Holmes wrote:
>>> On 27/02/2014 5:30 PM, Peter Levart wrote:
>>>> On 02/27/2014 08:29 AM, Peter Levart wrote:
>>>>> On 02/26/2014 09:54 PM, Martin Buchholz wrote:
>>>>>> I don't recall having added this particular wart
>>>>>> to test/java/lang/ProcessBuilder/Basic.java, and history suggests 
>>>>>> that
>>>>>> others did that.
>>>>>>
>>>>>> It does seem that being able to tell whether a java object 
>>>>>> monitor is
>>>>>> currently locked is useful for debugging and monitoring - there
>>>>>> should be a
>>>>>> way to do that.
>>>>>
>>>>> Thread.holdsLock(Object) ?
>>>>
>>>> Ah, you meant to query from some other thread, right?
>>>
>>> Right - that is the usage in Basic.java but I'm still scratching my
>>> head trying to understand what finding the BufferedInputStream locked
>>> is supposed to signify.
>>
>> Here's the relevant code block:
>>
>>                      while (unsafe.tryMonitorEnter(s)) {
>>                          unsafe.monitorExit(s);
>>                          Thread.sleep(1);
>>                      }
>>
>> This code block waits until it finds that some thread has acquired the
>> lock on 's' for at least 1 ms (or else it can miss it and will wait
>> further) ...
>>
>> Since this block is executed for s instanceof BufferedInputStream, what
>> this block does, is wait until one of synchronized methods on
>> BufferedInputStream is called. The thread that does that, calls read()
>> or read(byte[]) and blocks while holding a lock, since the child process
>> that is spawned and whose stdout the thread is trying to read in this
>> situation does a 10 minute sleep.
>
> Ah I see - thanks for the analysis. So this is a classic 'barrier' 
> waiting for the other thread to have reached the synchronized method.
>
>> I think the following could be used instead for the same purpose:
>>
>>              while (!isLocked(s, 100L)) {
>>                  Thread.sleep(1);
>>              }
>>
>> // using the following utility method:
>>
>>      static boolean isLocked(final Object monitor, long millis) throws
>> InterruptedException {
>>          Thread t = new Thread() {
>>              @Override
>>              public void run() {
>>                  synchronized (monitor) { }
>>              }
>>          };
>>          t.start();
>>          t.join(millis);
>>          return t.isAlive();
>>      }
>
> Functionally yes this will only return true if the object is locked 
> for >100ms. But it is potentially going to generate a lot of temporary 
> threads. And in theory at least the JIT can optimize the run method 
> into nothing.

Ok, then what about the following:

     static boolean isLocked(final Object monitor, final long millis) 
throws InterruptedException {
         return new Thread() {
             volatile boolean unlocked;

             @Override
             public void run() {
                 synchronized (monitor) { unlocked = true; }
             }

             boolean isLocked() throws InterruptedException {
                 start();
                 join(millis);
                 return !unlocked;
             }
         }.isLocked();
     }


// and usage it like:

             while (!isLocked(s, 100L)) {
                 Thread.sleep(100L);
             }


This will allocate about 10 Thread objects per second until the other 
thread finally reaches the synchronized read() method in 
BufferedInputStream...


Regards, Peter

>
> Still it is a cute trick :)
>
> David
>
>>
>> Regards, Peter
>>
>>>
>>> David
>>> -----
>>>
>>>> Peter
>>>>
>>>>>
>>>>> Regards, Peter
>>>>>
>>>>>>
>>>>>>
>>>>>> On Wed, Feb 26, 2014 at 7:12 AM, Paul Sandoz 
>>>>>> <paul.sandoz at oracle.com>
>>>>>> wrote:
>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> Out of all the methods on Unsafe i think the
>>>>>>> monitorEnter/monitorExit/tryMonitorEnter are the least used and are
>>>>>>> very
>>>>>>> strong candidates for removal.
>>>>>>>
>>>>>>> 99% of use-cases are supported by classes in the
>>>>>>> java.util.concurrent.locks package.
>>>>>>>
>>>>>>>
>>>>>>> Within the JDK itself it is only used in one JDK test file
>>>>>>> test/java/lang/ProcessBuilder/Basic.java:
>>>>>>>
>>>>>>>                      while (unsafe.tryMonitorEnter(s)) {
>>>>>>>                          unsafe.monitorExit(s);
>>>>>>>                          Thread.sleep(1);
>>>>>>>                      }
>>>>>>>
>>>>>>> for a test verifying an EOF is received on pending reads and it is
>>>>>>> polling
>>>>>>> to check when the process builder acquires the lock before
>>>>>>> destroying the
>>>>>>> process, presumably to avoid some sort of race condition that
>>>>>>> occasionally
>>>>>>> causes the test to fail.
>>>>>>>
>>>>>>> I believe this test as been through a number of rounds, i stared 
>>>>>>> at     public static boolean isLocked(final Object monitor, 
>>>>>>> final long millis) throws InterruptedException {
>>>>>>>         return new Thread() {
>>>>>>>             volatile boolean unlocked;
>>>>>>>
>>>>>>>             @Override
>>>>>>>             public void run() {
>>>>>>>                 synchronized (monitor) { unlocked = true; }
>>>>>>>             }
>>>>>>>
>>>>>>>             boolean isLocked() throws InterruptedException {
>>>>>>>                 start();
>>>>>>>                 join(millis);
>>>>>>>                 return !unlocked;
>>>>>>>             }
>>>>>>>         }.isLocked();
>>>>>>>     }
>>>>>>>
>>>>>>> things
>>>>>>> for a bit, but cannot quickly work out a replacement; i lack the
>>>>>>> knowledge
>>>>>>> on this area.
>>>>>>>
>>>>>>>
>>>>>>> Outside of the JDK i can only find one usage of monitorExit/Enter
>>>>>>> (according to grep code) in JBoss modules, and i believe this is
>>>>>>> only used
>>>>>>> on Java 1.6 to work around some limitations in class loading that
>>>>>>> were
>>>>>>> fixed in 1.7.
>>>>>>>
>>>>>>>
>>>>>>> Given such very limited use i propose to remove these methods after
>>>>>>> having
>>>>>>> worked out a fix for ProcessBuilder/Basic.java test.
>>>>>>>
>>>>>>> Paul.
>>>>>>>
>>>>>
>>>>
>>