Locking on primitive boxes

[Alan Malloy]

I had a look through Google's codebase, to see if we have any outlaws who
are using these primitive boxes as locks. Of course, almost all locks (over
99.9% in Google's codebase) have a static type of either Object (typically
a new Object() created just for its lock), or Class (typically for
class-wide exclusion during a static method/initializer). It's not easy for
me to discover how often these Objects are actually Integers, but what I
could do was at least find cases where an Integer is explicitly used as a
lock, to see if any of those are intentional, important use cases that
might be worth changing Brian's proposal for.

The TL;DR is: We do have a small number of cases using boxes as locks, but
none of them are a good idea, and there are easy "workarounds" available if
locking on a box became illegal (and which should really be applied
anyway). I don't foresee any serious migration issues that we would incur
based on these cases - through again, for Integers whose static type is
Object, I have no data at this time, and we would probably appreciate some
gradual-migration tools like a warning if the VM notices we're taking an
ill-advised lock.

For more information about how I came to this conclusion, read on.

I searched for all statements of the form: synchronized (expr()) {...}, or
a use of wait/notify/notifyAll on an expr(), and asked javac what the
static type of expr() is. The only primitive-box types that appear in this
list are Boolean, Integer, and Long. The Boolean cases that I looked at all
have the same basic structure: someone wants to store a boolean field, but
wants to update it in a critical section. Instead of doing the "right"
thing and using a separate lock, such as `this` or a dedicated lock object,
they just turn their boolean into a Boolean and lock on it while updating
it:

class Flag {
  Boolean active = false;
  void update() {
    synchronized (active) { active = computeNewState(); }
  }
}

This is clearly wrong, and it's easy to fix.

There were a slightly more varied set of patterns around Integer locks. I
identified 3 basic categories of motivations.

One amusing category is "wrong on purpose", as part of some
demonstration of a tool that detects locking problems. If this ever becomes
illegal, we can just update the example to use an Object instead:
      final Integer dummy = new Integer(0);
      synchronized (dummy) {
        runExample("blocked", new Runnable() {
          public void run() {
            synchronized (dummy) {
            }
          }
        });
        sleep(2000);
      }

A second, fairly innocuous, case is from authors who seem unaware that you
can use Class or Object for locks. Code in this category creates an Integer
for the sole purpose of locking on it, and never reads or writes the field
otherwise. In one example, the author notes that they wish they could use
new Object() instead, but their serialization framework requires all fields
to be serializable. Integer must have looked like an easy solution, but we
know there were better options available. A simple example of a class in
this category:
class Counters {
  private Map<String, String> values;
  private final Integer valuesLock = 0;
  void count(String k, String v) {
    synchronized(valuesLock) {
      values.put(k, v);
    }
  }
}

All the above cases share the problem that they introduce unnecessary
contention with other objects making the same mistake. The third category I
found is worse, because it also has the problem of not ensuring mutual
exclusion! The Integer being locked on is reassigned while the lock is
held, meaning that other threads could barge in, since they will now be
locking on a different object. An example:
class Sampler {
  private Integer count = 0;
  private final int frequency = 100;
  void maybeEmit(Object sample) {
    synchronized(count) {
      count++;
    }
    if (count % frequency == 0) {
      emit(sample);
    }
  }
}

On Wed, Feb 19, 2020 at 8:46 AM Brian Goetz <brian.goetz at oracle.com> wrote:

> One of the main impediments to migrating the primitive wrappers to be
> the reference projection of an inline class is locking -- there may be
> code out there that locks on instances of primitive wrappers.  Let's
> look at how we might wean users off, and what the compatibility
> constraints really are.
>
> First, note that all the wrappers have had static `valueOf()` factories,
> and the corresponding constructors have been deprecated since Java 9.
> It would be reasonable to deprecate these for removal at any time.
> (These would not actually be removed, but instead made private.)
>
> Having removed the public constructors, now the only way to get your
> hands on an Integer is through some mechanism like `valueOf()`.  I claim
> now that:
>
>      Under such a scheme, any code outside of java.lang that
> synchronizes on a wrapper is
>      inherently broken
>
> That is, it is synchronizing on an object whose locking protocol you are
> not party to, which is not guaranteed non-aliased, and which may in fact
> be permanently locked by some other thread. Therefore, it is a
> forseeable outcome for such a sync attempt to block forever.
>
> This was the intent behind https://openjdk.java.net/jeps/169, though it
> wasn't obvious from the title -- the ability to mark certain objects as
> permanently locked.
>
> So, concrete proposal:
>
>   - Move forward with the deprecation-for-removal of the constructors;
>
>   - Add hortatory notes in the spec for `valueOf()` that locking on
> objects whose provenance you do not control is a risky business, and may
> well never complete;
>
>   - Poison instances of primitive wrappers by marking their headers at
> creation with a bit pattern that will send locking through the slow
> path, where it can see that these are poisoned objects and cause lock
> attempts to never complete (or throw)
>
>   - Provide a JDK-scoped option to turn off the above behavior
> (-Xx:AllowStupidBrokenLockingOnPrimitiveWrappers)
>
> I don't see anything stopping us from doing any of these immediately.
>
>
>
>