1

I came across code like this in one of the repos. I checked it out and it works. (Only one thread enters the synchronized block.)

public Void hello(String s) {

    synchronized (s) {

        i++;
        System.out.println(i);
    }

    return null;
}

My question is is the lock obtained on the String class itself? If yes, wouldn't it mean if code like this exists else where in the code base, they will all wait to obtain a lock on the same object? Thus adding unnecessary delays?

Nathan Hughes
  • 94,330
  • 19
  • 181
  • 276
developer747
  • 15,419
  • 26
  • 93
  • 147
  • 3
    On the instance, just like for any other object. – akuzminykh Mar 03 '21 at 21:14
  • 1. It synchronizes on the `String` referred to by `s`. 2. Yes it would mean exactly that. So why do it? There is nothing inside the `synchronized` block that relies on `s`. – user207421 Mar 03 '21 at 22:45

1 Answers1

6

The intrinsic lock on the String object is the lock that gets acquired. But whether locking works depends on if the string is always the same instance or not. String pools and interning will affect this.

That it is difficult to figure out if the same instance will be used is only one reason not to do this.

If two classes in the application use the same string instance then one of them can acquire the lock and shut the other out. So you can have conceptually unrelated objects affecting each other, contending for the same lock.

Also people are going to be confused into thinking they can use the string value to mean something and have code change the value of s inside the synchronized block or method. That will break all the locking. Locks are on the objects, not on the variables. Changing values means the thread currently holding the lock now has the old object but threads trying to get in are trying to get the lock on the new object, a thread can acquire the new object and start executing the synchronized code before the previous thread is done.

It might work by accident sometimes but it is a terrible idea. Use a dedicated object as a lock instead:

private final Object lock = new Object();
Nathan Hughes
  • 94,330
  • 19
  • 181
  • 276
  • "[...] change the value inside the synchronized code." You mean of `s`? – akuzminykh Mar 03 '21 at 21:18
  • @akuzminykh: yes exactly. – Nathan Hughes Mar 03 '21 at 21:18
  • But you probably mean outside `synchronized`, right? Because `String` is immutable. Changing it within the block won't break the locking. Or I'm just misunderstanding something here. :) – akuzminykh Mar 03 '21 at 21:20
  • 1
    @akuzminykh: see this, it’s the same idea but with Booleans (also immutable of course): https://stackoverflow.com/a/10324280/217324. The strings are immutable but a variable can have one string swapped out for another. Chaos ensues. – Nathan Hughes Mar 03 '21 at 21:24
  • 2
    @akuzminykh when you have a construct like `synchronized(variable) { … }`, you are reading `variable` *before* entering the synchronized block (there’s no way around that). Hence, it doesn’t matter whether `variable` is changed inside or outside the synchronized block, as soon as there are concurrent modifications to `variable`, the read is racy and all bets are off. It doesn’t apply to the question’s code, as `s` is a local variable, but that’s actually worse. The caller could pass *anything* to the method and there’s no relationship between `s` and the variable `i` anyway. That’s just broken – Holger Mar 04 '21 at 11:33