How to avoid synchronization on a non-final field?

Question

If we have 2 classes that operate on the same object under different threads and we want to avoid race conditions, we'll have to use synchronized blocks with the same monitor like in the example below:

class A {
    private DataObject mData; // will be used as monitor

    // thread 3
    public setObject(DataObject object) {
       mData = object;
    }

    // thread 1
    void operateOnData() {
        synchronized(mData) {
            mData.doSomething();
            .....
            mData.doSomethingElse();
        }
    }
}

  class B {
    private DataObject mData;  // will be used as monitor

    // thread 3
    public setObject(DataObject object) {
       mData = object;
    }

    // thread 2
    void processData() {
        synchronized(mData) {
            mData.foo();
            ....
            mData.bar();
        }
    }
}

The object we'll operate on, will be set by calling setObject() and it will not change afterwards. We'll use the object as a monitor. However, intelliJ will warn about synchronization on a non-final field.

In this particular scenario, is the non-local field an acceptable solution?

Another problem with the above approach is that it is not guaranteed that the monitor (mData) will be observed by thread 1 or thread 2 after it is set by thread 3, because a "happens-before" relationship hasn't been established between setting and reading the monitor. It could be still observed as null by thread 1 for example. Is my speculation correct?

Regarding possible solutions, making the DataObject thread-safe is not an option. Setting the monitor in the constructor of the classes and declaring it final can work.

EDIT Semantically, the mutual exclusion needed is related to the DataObject. This is the reason that I don't want to have a secondary monitor. One solution would be to add lock() and unlock() methods on DataObject that need to be called before working on it. Internally they would use a Lock Object. So, the operateOnData() method becomes:

 void operateOnData() {
     mData.lock()
     mData.doSomething();
     .....
     mData.doSomethingElse();
     mData.unlock();
 }

Gowen

"Setting the monitor in the constructor of the classes and declaring it final can work." Sounds like the best option to me. – Canescent

I'll just have to use a separate monitor and object reference. I was wondering if there is another alternative or if I'm doing something wrong with the proposed architecture. – Gowen

Why do you even want to set the monitor object later? Without it being volatile you might indeed still run into issues. Try explaining more of the use case, this looks like there is a much better way of doing this with some dedicated concurrency tool. – Incorruptible

My exact use case is that DataObject is an audio/video muxer. Class A sends audio to it and Class B send video to it. The 2 classes exist before the muxer is instantiated. I need mutual exclusion when accessing the muxer. – Gowen

Pass DataObject in the constructor and set to a final field, as you suggested. – Asher

Forget about the "two classes". It doesn't matter how many classes are involved; If you have mutable data that are shared by more than one thread, then you likely need synchronization. Even if the shared data are private in just one class, you still need synchronization. – During

@james The reason that I mention that there are 2 (or more) classes is that the monitor object has to be passed on to both of them... I never said that synchronization is not needed. – Gowen

Keep in mind that it isn't "just a warning", synchronizing on a non-final field is a really bad idea. It is a great way to end up with threading bugs in the future. Just because you don't think the value will ever change doesn't mean that it is true. The only way to ensure that a value does not change is to declare it as final. This is why IntelliJ is warning you! If the value does change, the synchronization will no longer protect the code from multithreaded access. – Terpsichorean

Answer 1

You may create a wrapper

class Wrapper
{
    DataObject mData;

    synchronized public setObject(DataObject mData)
    {
        if(this.mData!=null) throw ..."already set"
        this.mData = mData;
    }

    synchronized public void doSomething()
    {
        if(mData==null) throw ..."not set"

        mData.doSomething();
    }

A wrapper object is created and passed to A and B

class A 
{
    private Wrapper wrapper; // set by constructor

    // thread 1
    operateOnData() 
    {
        wrapper.doSomething();
    }

Thread 3 also has a reference to the wrapper; it calls setObject() when it's available.

Diffractive

A wrapper wouldn't be ideal in my case, because each class performs different tasks on the data. Also the code complexity increases just to avoid a warning. I have edited my question to propose a possible solution for my case. – Gowen

Answer 2

A simple solution is to just define a public static final object to use as the lock. Declare it like this:

/**Used to sync access to the {@link #mData} field*/
public static final Object mDataLock = new Object();

Then in the program synchronize on mDataLock instead of mData.

This is very useful, because in the future someone may change mData such that it's value does change then your code would have a slew of weird threading bugs.

This method of synchronization removes that possibility. It also is really low cost.

Also having the lock be static means that all instances of the class share a single lock. In this case, that seems like what you want.

Note that if you have many instances of these classes, this could become a bottleneck. Since all of the instances are now sharing a lock, only a single instance can change any mData at a single time. All other instances have to wait.

In general, I think something like a wrapper for the data you want to synchronize is a better approach, but I think this will work.

This is especially true if you have multiple concurrent instances of these classes.

Terpsichorean

I have 2 different classes and they must use the same monitor. I could set it in their constructor. – Gowen

@Gowen In that case, you can possibly make the lock object public static final. Then multiple classes could use it. You would then be synchronized on a final variable and your warning would go away. I have never actually done this though, I am not sure if syncing on a static variable is different. I think it would still be OK as long as that static variable is final. – Terpsichorean

I thought about this a bit more and there is a difference, which in this case I think helps you. Using a static lock object means all instances of a class share a lock(since there is one static lock object they all share). That would be good in this case because of the cross class concurrency. – Terpsichorean

the 2 classes are different classes, not instances of the same class. – Gowen

@Gowen Right that's why you need static public. If something is static public is it accessible from any class. So, both classes could access it. The public keyword makes it visible to everything. The static keyword means there is one global instance of mDataLock. The final keyword means that instance is a constant and will never change. Together that gives you what you want. – Terpsichorean

I should mention that a wrapper is the best way to go here. I only proposed this because you didn't want to add any complexity. I honestly don't think a wrapper adds much complexity. – Terpsichorean

Answer 3

Some platforms provide explicit memory-barrier primitives which will ensure that if one thread writes to a field and then does a write barrier, any thread which has never examined the object in question can be guaranteed to see the effect of that write. Unfortunately, as of the last time I asked such a question, Cheapest way of establishing happens-before with non-final field, the only time Java could offer any guarantees of threading semantics without requiring any special action on behalf of a reading thread was by using final fields. Java guarantees that any references made to an object through a final field will see any stores which were performed to final or non-fields of that object before the reference was stored in the final field but that relationship is not transitive. Thus, given

class c1 { public final c2 f; 
           public c1(c2 ff) { f=ff; } 
         }
class c2 { public int[] arr; }
class c3 { public static c1 r; public static c2 f; }

If the only thing that ever writes to c3 is a thread which performs the code:

c2 cc = new c2();
cc.arr = new int[1];
cc.arr[0] = 1234;
c3.r = new c1(cc);
c3.f = c3.r.f;

a second thread performs:

int i1=-1;
if (c3.r != null) i1=c3.r.f.arr[0];

and a third thread performs:

int i2=-1;
if (c3.f != null) i2=c3.f.arr[0];

The Java standard guarantees that the second thread will, if the if condition yields true, set i1 to 1234. The third thread, however, might possibly see a non-null value for c3.f and yet see a null value for c3.arr or see zero in c3.f.arr[0]. Even though the value stored into c3.f had been read from c3.r.f and anything that reads the final reference c3.r.f is required to see any changes made to that object identified thereby before the reference c3.r.f was written, nothing in the Java Standard would forbid the JIT from rearranging the first thread's code as:

c2 cc = new c2();
c3.f = cc;
cc.arr = new int[1];
cc.arr[0] = 1234;
c3.r = new c1(cc);

Such a rewrite wouldn't affect the second thread, but could wreak havoc with the third.

Gorse

well, even more bizzare, in if(c3.r!=null){ ... c3.r ...} , the 2nd read of c3.r would see null! :) In theory you need to read it to a local variable r=c3.r; if(r!=null){...r...} to make sure the 2nd read sees the same value. – Diffractive

@bayou.io: I'm not sure if Java would be allowed to perform the second read before the first. It might be, but that would seem unusual. In any case, I think Java is needlessly inexpressive in its failure to provide a way to guarantee publication of a non-final field. – Gorse

it seems that a lot of changes are coming in java9 - openjdk.java.net/jeps/188 - openjdk.java.net/jeps/193 – Diffractive

@bayou.io: Variable handles sound cool. I wonder if Java will also add any mechanism for efficiently returning multiple values from a function? An approach which would be pretty useful relative to its cost would be to add a few few fields of various types to Thread and standardize rules for their use (e.g. specify that a function which would need to return two values of type int or long should use fields "retLong0" and "retLong1", and that anyone calling such a function should grab the values before calling any other function). Know if any such thing might get added? – Gorse