Synchronization on the local variables

Question

Hi community!

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!

Answer 1

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

Answer 2

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.

Answer 3

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
    }
}

Answer 4

It might be easier to work with a synchronized wrapper:

public class FooWrapper {
    public FooWrapper(Foo theFoo) {
        this.theFoo = theFoo;
        this.lock = new Object();
    }

    public void processIfRequired() {
        synchronized (lock) {
            if (theFoo.needsProcessing()) {
                theFoo.process();
            }
        }
    }

    private Foo theFoo;

    private Object lock;
}

then:

public void process(FooWrapper[] foos) {
    for (final FooWrapper foo : foos) {
        foo.processIfRequired();
    }
}

Answer 5

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. :-)

Answer 6

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?)

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).