Synchronization on the local variables

Question

I have a multithreaded Java code in which:

• several threads read stateful objects from the synchronized shared storage (i.e. as a result of this, some threads may reference the same objects);
• each thread then invokes a method process() and passes its object there;
• process() processes objects in some way, which may result changing the objects state;
• these state changes should be synchronized.

I've created a method like that:

public void process(Foo[] foos) {
    for (final Foo foo : foos) {
        if (foo.needsProcessing()) {
            synchronized (foo) {
                foo.process();  // foo's state may be changed here
            }
        }
    }
}

To the best of my knowledge, this looks legit. However, IntelliJ's inspection complains about synchronizing on the local variable because "different threads are very likely to have different local instances" (this is not valid for me, as I'm not initializing foos in the method).

Essentially what I want to achieve here is the same as having method Foo.process() synchronized (which is not an option for me, as Foo is a part of 3rd party library).

I got used to the code without yellow marks, so any advice from the community is appreciated. Is this really that bad to do synchronization on locals? Is there an alternative which will work in my situation?

Thanks in advance!

Malka

Per youtrack.jetbrains.com/issue/IDEA-116610 this is likely an IDE bug – Petrochemical

Answer 1

if (foo.needsProcessing()) {
    synchronized (foo) {
        foo.process();  // foo's state may be changed here
    }
}

I think there is a race condition in the above fragment that could result in foo.process() occasionally being called twice on the same object. It should be:

synchronized (foo) {
    if (foo.needsProcessing()) {
        foo.process();  // foo's state may be changed here
    }
}

---

Is this really that bad to synchronize on locals?

It is not bad to synchronize on locals per se. The real issues are:

• whether the different threads are synchronizing on the correct objects to achieve proper synchronization, and

• whether something else could cause problems by synchronizing on those objects.

Vachill

Right, thanks! BTW, what do you think on the main question here? Is this really that bad to synchronize on locals? – Malka

synchronized(foos) maybe better, because arrays can be modified too! foo only needs to be synchronized if not only used inside a synchronized(foos). Locking can be quite tricky to get right, as your corrections partially illustrate. If an object is protected by an object lock, all accesses must be via the same object lock, including the more tricky to use concurrent lock objects. – Riendeau

Answer 2

Stephen C's answer has problems, he's entering a lot of synchronization locks pointlessly, oddly enough the better way to format it is:

    public void process(Foo[] foos) {
        for (final Foo foo : foos) {
            if (foo.needsProcessing()) {
                synchronized (foo) {
                    if (foo.needsProcessing()) {
                        foo.process();  // foo's state may be changed here
                    }
                }
            }
        }
    }

Getting the synchronization lock can sometimes take a while and if it was held something was changing something. It could be something changed the needing-processing status of foo in that time.

You don't want to wait for the lock if you don't need to process the object. And after you get the lock, it might not still need processing. So even though it looks kind of silly and novice programmers might be inclined to remove one of the checks it's actually the sound way to do this so long as foo.needsProcessing() is a negligable function.

---

Bringing this back to the main question, when it is the case that you want to synchronize based on the local values in an array it's because time is critical. And in those cases the last thing you want to do is lock every single item in the array or process the data twice. You would only be synchronizing local objects if you have a couple threads doing a bunch of work and should very rarely need to touch the same data but very well might.

Doing this will only ever hit the lock if and only if, processing that foo that needs processing would cause concurrency errors. You basically will want double gated syntax in those cases when you want to synchronize based on just the precise objects in the array as such. This prevents double processing the foos and locking any foo that does not need processing.

You are left with the very rare case of blocking the thread or even entering a lock only and exactly when it matters, and your thread is only blocked for at exactly the point where not blocking would lead to concurrency errors.

Decastyle

Answer 3

IDE is supposed to help you, if it makes a mistake, you shouldn't bend over and please it.

You can disable this inspection in IntelliJ. (Funny it's the only one enabled by default under "Threading Issues". Is this the most prevalent mistake people make?)

Cobwebby

Answer 4

Refactor the loop-body into a separate method taking foo as parameter.

Romona

I'll need to synchronize on this method's parameter too – Malka

Yes, but it won't be a local variable then. – Gayton

IntelliJ has another type of inspection "different threads are very likely to have different method parameters" :) – Malka

How is a method parameter any different than a local variable? – Ahvenanmaa

Answer 5

No auto-intelligence is perfect. I think your idea of synchronizing on the local variable is quite correct. So perhaps do it your way (which is right) and suggest JetBrains that they should tune their inspection.

Stencil

Answer 6

There is nothing wrong with your synchronisation on an object within a collection. You might try replacing the foreach with a normal for loop:

for (int n = 0; n < Foos.length; n++) {
    Foo foo = Foos[n];

    if (null != foo && foo.needsProcessing()) {
        synchronized (foo) {
            foo.process();  // foo's state may be changed here
        }
    }
}

or even (so the detector won't trip over the Foo foo):

for (int n = 0; n < foos.length; n++) {
    if (null != foos[n] && foos[n].needsProcessing()) {
        synchronized (foos[n]) {
            foos[n].process();  // foos[n]'s state may be changed here
        }
    }
}

Not using a temporary value to prevent the multiple foos[n] isn't best practice, but if it works to prevent the unwanted warning you might live with it. Add a comment why this code has a deviant form. :-)

Odontoid

Answer 7

In the .NET world objects sometimes carry their lock object as property.

synchronized (foo.getSyncRoot()) {
    if (foo.needsProcessing()) {
        foo.process();  // foo's state may be changed here
    }
}

This allows the object to give out a different lock object, depending on its implementation (e.g. delegating the monitor to a underlying database connection or something).

Romona