Multiple object locks in Java?

Question

Is it safe/acceptable practice to lock on a private field variable (instead of using a lock object)? This way, I could have different locks for different purposes. Example below:

class Test {
  private Integer x = 0;
  private Integer y = 0;

  public void incrementX() {
    synchronized(x) {
      x++;
    }
  }

  public void decrementX() {
    synchronized(x) {
      x++;
    }
  }

  public void incrementY() {
    synchronized(y) {
      y++;
    }
  }

  public void decrementY() {
    synchronized(y) {
      y++;
    }
  }

Or should I have a lock object for each private member I wish to lock? Example:

class Test {
  private final Object xLock = new Object();
  private final Object yLock = new Object();
  private Integer x = 0;
  private Integer y = 0;

...

}

Or should I just have a single general lock and use that for all private variables that require locking? Example:

class Test {
  private final Object objLock = new Object();
  private Integer x = 0;
  private Integer y = 0;

...

}

Answer 1

Beware to always use a final member var for the lock! If you use an Integer, for example, and you plan to change it, that will be very bad practice since each call will see a different object and cause a data race.

Whether you use one or several locks depends on the coordination scheme you want to achieve, so it's entirely domain-specific. You must think through carefully which operations are and which aren't mutually exclusive and assign locks to them appropriately. There is no single best practice here.

If you have two orthogonal operations on your object that may happen simultaneously without causing any datarace, that's a case for two locks. In your example there are two Integers, each changing independently. I see this as a case for two locks. If you had more complex code where in at least one operation you needed to access both Integers, that would tie them together and then you would need a single lock.

Answer 2

It's perfectly acceptable to lock on a private field, as long as this field is an object. Primitives don't have an intrinsic lock, and the first snippet is thus invalid.

I would however avoid locking on a private field if this field is accessible from the outside (using a getter, for example), as this would allow anyone to lock on the same object for different purposes. The second solution is thus the cleanest, IMHO.

Using a single lock is counter-productive, since it prevents concurrent access to methods which should be able to run concurrently. It's thus generally better to have fine-grained the locks.

EDIT:

now that you have changed your question and use wrapper objects, the locking on the private Integer instances is really not a good solution, as you change the values of these variables inside the methods. Use final fields as locks.

Remember that x++, if x is an Integer instance, is equivalent to:

int temp = x.intValue();
temp++;
x = Integer.valueOf(temp);

Moreover, since Integer.valueOf() caches Integer instances, you might have several classes using the same Integer instance to lock completely different things. A recipe for slow execution and deadlocks.