synchronized on a variable

[Linwood Hayes]

public class TestClass {

  private int number;
  private String series;

  public void testIt() {

      synchronized(series) {
        series = ...
      }

       synchronized(Integer.toInt(number)) {
          number = ....
       }
  }

}

Does the    synchronized(Integer.toInt(number)) achieve the supposed synchronization effect on variable "number" ?

[Mike Simmons]

Linwood Hayes wrote:Does the    synchronized(Integer.toInt(number)) achieve the supposed synchronization effect on variable "number" ?

Well, there's no method Integer.toInt(), so we would have to guess what it does.  I have a few different guesses which would lead to different results.  I suggest you try compiling the code to see if it is even allowed, then let us know what you mean to ask about.

[Linwood Hayes]

Mike Simmons wrote:

Linwood Hayes wrote:Does the    synchronized(Integer.valueOf(number)) achieve the supposed synchronization effect on variable "number" ?

Well, there's no method Integer.toInt(), so we would have to guess what it does.  I have a few different guesses which would lead to different results.  I suggest you try compiling the code to see if it is even allowed, then let us know what you mean to ask about.

sorry, I didn't think when I typed..  have corrected it in the code.  it is "valueOf()"

[Mike Simmons]

OK.  So it returns an Integer, while the original variable number was an int.  On one level, yes it is possible to synchronize on Integer.valueOf(number).  But that's not the same thing as synchronizing on number, which is a different type, and not possible to synchronize on because it's not an Object.

Regardless, it's a bad idea for a few different reasons.  One is that I can't even tell what the goal would be for synchronizing like this.  Another is that we don't know whether valueOf() will return a shared cached instance, or a unique value.  So it's hard to predict what effect this will have.  And also because code really shouldn't synchronize on any object it doesn't own or control in some sense... because just about any library out there could have access to the same Integer instances that we do, and if that code also synchronizes on those instances, it can create deadlock.  Don't synchronize on an object that you don't know where it came from.

[Linwood Hayes]

Thank you for the input

[Rob Spoor]

Synchronizing on the result of Integer.valueOf (and valueOf methods in the other numeric wrappers) has one of two results:

1) If the value is in the cache (by default: -128 to 127; for Integer this can be extended with a system property), then you synchronize on the value that is used all over the place. It's not "your" value. This can lead to several issues, including deadlock (due to lock ordering, which you cannot control because everybody can synchronize on the value).
2) If the value is not in the cache, you synchronize on a newly created object. The synchronization is senseless, and the JVM (or even the compiler) may omit the synchronization completely.

If you need to synchronize on a primitive field, you need a separate lock for that. It can be an existing field, or a completely new field (e.g. private final Object numberLock = new Object();). Or, if possible, don't use int but AtomicInteger instead, if the field does not take part in any invariant that involves other fields. If it does, you need to use one single lock for updating all these fields anyway.

[Campbell Ritchie]

If you are using locks, wouldn't you want to use an explicit lock object? Try the java.util.concurrent.locks package.

class Foo
{
   private final Lock lock = new ReentrantLock();
   // ...
   public void foo(...)
   {
      lock.lock();
      try
      {
          // ...
      }
      finally
      {
          lock.unlock();
      }
   }
   // ...
}

It is possible to lock the same Lock repeatedly as long as it is unlocked every time it is used. There are also other kinds of Lock.

[Rob Spoor]

It depends on your needs. I recently re-read Java Concurrency in Practice, and it doesn't say you should drop all intrinsic locks. It even states that it's sometimes the best solution.

If all locking and unlocking is done as part of the same block like your example, and I only have one condition, I prefer intrinsic locks over ReentrantLock because I don't have to add all the try-finally overhead. As soon as I have multiple conditions, or I need to use more complex flows for lock+unlock (for whatever reason, it hasn't happened so far fortunately), then Reentrantock is the way to go for me.

[Linwood Hayes]

What if

public class TestClass {
 
  private Integer number;
  private String series;
 
  public void testIt() {
 
      synchronized(series) {
        series = ...
      }
 
       synchronized(number)) {
          if(number.intValue() == 2) 
             number += ...;
       }
  }
 
}

in this case, regardless of whether it is a good practice, such synchronization functions on the "number" var, right ?

[Rob Spoor]

Yes, but unfortunately, the synchronizing is done on different objects. When you do number += ..., what actually happens is this:

int tmp = number; // auto-unboxing
tmp += ...;
number = tmp; // auto-boxing

That means that you get the same issue as synchronizing on Integer.valueOf(number) - it still isn't "your" object. If this is your case, I suggest using AtomicInteger instead:

public class TestClass {
 
  private final AtomicInteger number = new AtomicInteger();
  private String series;
 
  public void testIt() {
 
      synchronized(series) {
        series = ...
      }

      number.updateAndGet(num -> num == 2 ? num + ... : num);
  }
}

You can also use getAndUpdate; the difference is in whether you get the old (getAndUpdate) or new (updateAndGet) value. To prevent the notorious check-then-act issue, where the value can have changed after the checking, the check is moved to inside the updating.

Come to think of it, because you only add a value if the current value is 2, you can use compareAndSet:

number.compareAndSet(2, 2 + ...);

[Rob Spoor]

Come to think about it some more, the synchronization on series is also wrong. Again, you switch the objects you synchronize on. And again, it's not necessarily "your" object. The string may be a constant like "hello", which means it comes from the string pool. That in turn means that every other use of the same constant can synchronize on the same object, with the same problems described earlier.

In this case:

synchronize on this

or introduce a new (private final) lock field

or use an AtomicReference<String>

[Mike Simmons]

Well, with "series", we have no idea how or if it's being initialized.  As written, it's not assigned to until after the JVM attempts to synchronize on it.  Which will result in a NullPointerException since it's null.