Unsafe: removing the monitorEnter/monitorExit/tryMonitorEnter methods

[Peter Levart]

On 03/04/2014 05:09 AM, David Holmes wrote:
> On 4/03/2014 1:45 PM, David Holmes wrote:
>> On 3/03/2014 10:56 PM, David M. Lloyd wrote:
>>> Yes, that would necessarily be the contract of a Monitors class, 
>>> just as
>>> it is part of the contract of Lock today.  If your argument is that it
>>> shouldn't be allowed because it might be used wrong, we might as well
>>> just delete most of the JDK, ReentrantLock included, since it suffers
>>> from the exact same potential problem.  The difference is that monitors
>>> have a simple API in the form of synchronized that people were in the
>>> past and would continue to be free (and recommended) to use.
>>
>> We should not introduce anything that allows something that was
>> guaranteed to be safe by the language, to become unsafe. So I can
>> support a call for tryMonitorEnter, but not for explicit enter/exit
>> actions.
>
> Which of course is not possible at the API level - Doh! Sorry.

Unless the API is made safe by packing it into a lambda-consuming 
method. For example:

public static boolean trySynchronized(Runnable action);

Regards, Peter

>
> David
>
>> David
>> -----
>>
>>> I would not call recommending monitors to be a "premature 
>>> optimization".
>>>   The memory overhead of ReentrantLock is far too large for many use
>>> cases, especially when it comes to fine-grained locking of large data
>>> sets.  In fact I would *only* recommend Lock if your need for the extra
>>> behavior it provides (i.e., multiple conditions, tryLock, timed 
>>> tryLock,
>>> and/or interruptible locking) or for the extra behavior provided by
>>> ReentrantLock itself (i.e., lock status introspection) outweighs the
>>> cost of its memory consumption, which so far has happened zero times in
>>> any of the many frameworks I maintain.
>>>
>>> On the other hand, adding some of the missing functionality to a
>>> Monitors class would be pretty handy, especially if doing so doesn't
>>> create a large divergence from the existing implementation. Given that
>>> tryMonitorEnter already exists, implementing that method on a Monitors
>>> class seems fairly trivial, which already takes care of one out of the
>>> four special use cases for Locks.  Interruptible locking would be my
>>> first choice for a number two, if this idea were actually officially on
>>> the table.  Finally, an isMonitorHeld() method would tie in another
>>> branch of the discussion nicely and seems trivially possible as well.
>>> Any other information would be a purely
>>>
>>> It's worth noting that it's already possible for user code to implement
>>> the basic functionality using JNI methods, though in the interest of
>>> full disclosure it's worth noting that the enter/exit monitor JNI
>>> functions are forbidden by spec to interoperate with the corresponding
>>> bytecodes.
>>>
>>> FWIW using the word "monitor" instead of "lock" may help avoid 
>>> confusion
>>> and "scare off" those pesky undergrads.
>>>
>>> On 03/02/2014 12:48 PM, Dr Heinz M. Kabutz wrote:
>>>> Hi Dave,
>>>>
>>>> "lighter" is subject to the HotSpot Profiler doing the optimizations
>>>> that you expect.  Problem is, once you begin manually locking /
>>>> unlocking monitors, the byte codes are not recognizable as standard
>>>> synchronized(monitor) {} code, thus the optimizations will also not be
>>>> as good (or completely absent).  At least that has been my experience
>>>> when I tried it out a few years ago.  So the argument that 
>>>> synchronized
>>>> is faster than ReentrantLock, besides this being IMHO a premature
>>>> optimization, cannot be directly applied to when we manually do use 
>>>> the
>>>> monitorEnter/monitorExit methods.
>>>>
>>>> The reason why I personally think they are more dangerous than
>>>> ReentrantLock is that we are used to synchronized() {} taking care of
>>>> the unlocking for us.  If we see a thread that is in the BLOCKED state
>>>> (which only happens with synchronized) then we would typically assume
>>>> that another thread is /currently /holding the monitor.
>>>>
>>>> Take the following example code:
>>>>
>>>> import sun.misc.*;
>>>>
>>>> import java.lang.reflect.*;
>>>>
>>>> public class MonitorMystery {
>>>>    public static Unsafe getUnsafe() {
>>>>      try {
>>>>        for (Field field : Unsafe.class.getDeclaredFields()) {
>>>>          if (Modifier.isStatic(field.getModifiers())) {
>>>>            if (field.getType() == Unsafe.class) {
>>>>              field.setAccessible(true);
>>>>              return (Unsafe) field.get(null);
>>>>            }
>>>>          }
>>>>        }
>>>>        throw new IllegalStateException("Unsafe field not found");
>>>>      } catch (Exception e) {
>>>>        throw new IllegalStateException(
>>>>            "Could not initialize unsafe", e);
>>>>      }
>>>>    }
>>>>
>>>>    public static void main(String... args) throws 
>>>> InterruptedException {
>>>>      Thread t = new Thread() {
>>>>        public void run() {
>>>>          getUnsafe().monitorEnter(MonitorMystery.class);
>>>>        }
>>>>      };
>>>>      t.start();
>>>>      Thread.sleep(100);
>>>>      System.out.println("Trying to synchronize");
>>>>      synchronized (MonitorMystery.class) {
>>>>        System.out.println("Managed to synchronized");
>>>>      }
>>>>    }
>>>> }
>>>>
>>>> We now see that we never manage to synchronize in the main thread 
>>>> and in
>>>> fact if you do a thread dump you will see a deadlock involving only a
>>>> single thread :-)
>>>>
>>>> This is why it is so important to use the correct idioms. In my
>>>> experience, there are very few Java programmers who get this right.
>>>> Brian Goetz's book is correct, but there are other books out there 
>>>> that
>>>> do a shocking job of mangling the idiom.
>>>>
>>>> Whenever possible, use synchronized() { }.  If not, IMHO it would be
>>>> preferable to switch to ReentrantLock.
>>>>
>>>> Regards
>>>>
>>>> Heinz
>>>> -- 
>>>> Dr Heinz M. Kabutz (PhD CompSci)
>>>> Author of "The Java(tm) Specialists' Newsletter"
>>>> Oracle Java Champion 2005-2013
>>>> JavaOne Rock Star Speaker 2012
>>>> http://www.javaspecialists.eu
>>>> Tel: +30 69 75 595 262
>>>> Skype: kabutz
>>>>
>>>>
>>>>
>>>> David M. Lloyd wrote:
>>>>> Making the language Kindergarten-friendly at the cost of general
>>>>> usefulness is a mistake, IMO.  And anyway there's nothing that is 
>>>>> less
>>>>> safe about a Monitors class than ReentrantLock; on the other hand,
>>>>> monitors continue to be considerably lighter (size and (for most of
>>>>> the history of JUC) speed) by every measurement I've ever made.  I
>>>>> would advise monitors over ReentrantLock 9 times out of 10 in any of
>>>>> our code.
>>>>>
>>>>> I just don't think your metaphors - neither of monitor methods being
>>>>> dangerous, nor of Java developers being infants - are really apt.
>>>>>
>>>>> On 03/02/2014 02:51 AM, Dr Heinz M. Kabutz wrote:
>>>>>> With a curious 9 months old crawling around the house, I've just 
>>>>>> moved
>>>>>> the sharp knives to the top draw in the kitchen - out of reach.  I
>>>>>> don't
>>>>>> think we should be encouraging people to use monitor.tryLock() for
>>>>>> various reasons:
>>>>>>
>>>>>> 1. We have a richer interface with Lock/ReentrantLock, including
>>>>>> better
>>>>>> Condition that allow easier timed wait idioms.
>>>>>>
>>>>>> 2. It is just too easy to get the idioms wrong for Lock.lock() and
>>>>>> Lock.unlock().  Every time I show this idiom some people in the
>>>>>> audience
>>>>>> start arguing with me:
>>>>>>
>>>>>> lock.lock();
>>>>>> try {
>>>>>>   // do something
>>>>>> } finally {
>>>>>>   lock.unlock();
>>>>>> }
>>>>>>
>>>>>> IMHO, this is really an edge case that might be useful to have
>>>>>> semi-accessible at some point, but not something that should 
>>>>>> generally
>>>>>> be done.  It belongs in the same draw as the sharp knives and the
>>>>>> ability to cause arbitrary asynchronous exceptions (which has been
>>>>>> made
>>>>>> more difficult to do in Java 8).
>>>>>>
>>>>>> Brian Goetz wrote:
>>>>>>> Except that Lock has features that are not supported by intrinsic
>>>>>>> locks (timed wait, interruptible wait.)  So the Lock returned would
>>>>>>> not conform to Lock's contract, and attempting to call these 
>>>>>>> methods
>>>>>>> would probably throw UOE.
>>>>>>>
>>>>>>> On 2/27/2014 6:12 AM, Stephen Colebourne wrote:
>>>>>>>> On 26 February 2014 20:54, Martin Buchholz <martinrb at google.com>
>>>>>>>> wrote:
>>>>>>>>> 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.
>>>>>>>>
>>>>>>>> Perhaps it would be useful to be able to expose a java object
>>>>>>>> monitor
>>>>>>>> as an instance of Lock?
>>>>>>>>
>>>>>>>> Lock lk = Lock.ofMonitor(object)
>>>>>>>> if (lk.tryLock()) {
>>>>>>>>    ...
>>>>>>>> }
>>>>>>>>
>>>>>>>> Such a method feels like it would be a useful missing link between
>>>>>>>> synchronized and locks.
>>>>>>>>
>>>>>>>> Stephen
>>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>>
>>>