Unsafe: removing the monitorEnter/monitorExit/tryMonitorEnter methods

[David Holmes]

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.

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
>>>>>>>
>>>>>>
>>>>
>>>>
>>
>>