As jontro already mentioned in his answer (and basically, as the warning already says) : This sort of synchronization on the ShortCircuit object does not have the effect that the developer probably hoped to achieve. Unfortunately, the tooltip in your screenshot hides the actual code, but it seems like the code could roughly be
synchronized (s)
{
if (!s.isFinalCallbackFired())
{
s.doFire();
}
}
That is: It is first checked whether isFinalCallbackFired returns false, and if this is the case, something (hidden) is done, which likely causes the isFinalCallbackFired state to switch to true.
So my assumption is, roughly, that the goal of putting the if statement into the synchronized block was to make sure that doFire is always called exactly once.
And indeed, at this point, the synchronization could be justified. More specifically, and a bit oversimplified:
What can be guaranteed:
When two threads are executing the fireFinalCallback method with the same ShortCircuit parameter, the synchronized block will guarantee that only one thread at a time can check the isFinalCallbackFired state and (if it is false) call the doFire method. So it is guaranteed that doFire will be called only once.
What can not be guaranteed:
When one thread is executing the fireFinalCallback method, and another thread does any operation on the ShortCircuit object (like calling doFire), then this might lead to an inconsistent state. Particularly, if another thread also does
if (!s.isFinalCallbackFired())
{
s.doFire();
}
but without synchronizing on the object, then doFire may be called twice.
The following is an MCVE that illustrates the effect:
public class SynchronizeOnParameter
{
public static void main(String[] args)
{
System.out.println("Running test without synchronization:");
runWithoutSync();
System.out.println();
System.out.println("Running test with synchronization:");
runWithSync();
System.out.println();
System.out.println("Running test with wrong synchronization:");
runWithSyncWrong();
System.out.println();
}
private static void runWithoutSync()
{
ShortCircuit s = new ShortCircuit();
new Thread(() -> fireFinalCallbackWithoutSync(s)).start();
pause(250);
new Thread(() -> fireFinalCallbackWithoutSync(s)).start();
pause(1000);
}
private static void runWithSync()
{
ShortCircuit s = new ShortCircuit();
new Thread(() -> fireFinalCallbackWithSync(s)).start();
pause(250);
new Thread(() -> fireFinalCallbackWithSync(s)).start();
pause(1000);
}
private static void runWithSyncWrong()
{
ShortCircuit s = new ShortCircuit();
new Thread(() -> fireFinalCallbackWithSync(s)).start();
if (!s.isFinalCallbackFired())
{
s.doFire();
}
}
private static void fireFinalCallbackWithoutSync(ShortCircuit s)
{
if (!s.isFinalCallbackFired())
{
s.doFire();
}
}
private static void fireFinalCallbackWithSync(ShortCircuit s)
{
synchronized (s)
{
if (!s.isFinalCallbackFired())
{
s.doFire();
}
}
}
static class ShortCircuit
{
private boolean fired = false;
boolean isFinalCallbackFired()
{
return fired;
}
void doFire()
{
System.out.println("Calling doFire");
pause(500);
fired = true;
}
}
private static void pause(long ms)
{
try
{
Thread.sleep(ms);
}
catch (InterruptedException e)
{
e.printStackTrace();
}
}
}
The output is
Running test without synchronization:
Calling doFire
Calling doFire
Running test with synchronization:
Calling doFire
Running test with wrong synchronization:
Calling doFire
Calling doFire
So the synchonized block does make sure that the doFire method is only called once. But this only works if all modifications are only done in the fureFinalCallback method. If the object is modified elsewhere, without a synchronized block, the doFire method may be called twice.
(I'd like to offer a solution for this, but without details about the ShortCircuit class and the remaining classes and processes, one could only give the vague hint to have a look at the java.util.concurrent package and its subpackages: Locks and Conditions might be a viable path, but you have to figure that out...)
