CODEWITHSUNDEEP
javamultithreading
craigmiller160
craigmiller160

Reputation: 6273

Java Thread Safety: How to handle an instance variable when you have to call methods on it

So I'm pretty good overall when it comes to the thread-safety of code, but I'm currently encountering a situation where I'm not clear on the best way to handle it.

I have an instance variable, non-final because the enclosing class can change its value. This variable is an object that has several methods that get called on it during the application. Here is a quick sample to show what I mean.

private class Foo{

        private FooFoo fooFoo;

        public synchronized void setFooFoo(FooFoo fooFoo){
            this.fooFoo = fooFoo;
        }

        public void doSomething(){
            fooFoo.doSomething(); //How do I make this line thread-safe?
        }

    }

Changing the reference of the fooFoo field is easy, just simple synchronization. But what about when the doSomething() method is called on fooFoo? I'm always hesitant to synchronize on an alien method due to deadlock risk.

In the real cases this is based on, there are many different variations of this. My company's application is a sprawling code base that frequently resembles a bowl of spaghetti, so when it comes to writing any kind of synchronized code I'm extra paranoid, because of the tight coupling and the fact that there are developers not only here in the US, but in an offshore firm in eastern europe working on it and I do not trust all of them to make good coding decisions.

So I'm just looking for the best practice to handle this kind of situation in a multi-threaded environment. Thanks.

Upvotes: 2

Views: 1041

Answers (3)

cyroxis
cyroxis

Reputation: 3711

It depends upon what your concerns are for thread safety.

If foo is only delegated to you can simply make it volatile. This will prevent threads from cashing a reference to the old value if the reference is updated. FooFoo can then handle it's own thread safety concerns. 

private class Foo{

    private volatile FooFoo fooFoo;

    public void setFooFoo(FooFoo fooFoo){
        this.fooFoo = fooFoo;
    }

    public void doSomething(){
        fooFoo.doSomething();
    }

}

If your concern is about thread safety of Foo itself, and it is doing more then just delegating calls you should synchronize relevant methods. 

private class Foo{

    private FooFoo fooFoo;

    public synchronized void setFooFoo(FooFoo fooFoo){
        this.fooFoo = fooFoo;
    }

    public synchronized void doSomething(){
        fooFoo.doSomething();
    }

    public synchronized void doSomethingElse() {
        int values = fooFoo.getValue();
        // do some things
        fooFoo.setValue(values + somethingElse);
    }
}

Upvotes: 0

Solomon Slow
Solomon Slow

Reputation: 27190

fooFoo.doSomething(); //How do I make this line thread-safe?

Hint: You can't make that one line thread-safe unless that is the only line in the whole program that ever accesses the object.

Thread-safety is not about making particular lines of code or particular methods thread safe: It's about making data thread safe.

Does fooFoo refer to a mutable object? If not, then that line already is thread safe. But if the object is mutable, then thread-safety, at a minimum, means insuring that unintended interactions between two or more threads can not put that object into an invalid state; and at the worst case it means insuring the consistency of relationships between the fooFoo object and other objects in your program.

Any time there is an important relationship between two or more pieces of data that are shared between threads, then you probably need to throw a synchronized block around any bit of code that could temporarily violate that relationship, and you need to throw a synchronized block around any bit of code that depends on that relationship---even if the code only looks at the data.

Upvotes: 2

midor
midor

Reputation: 5557

In your case you would have to make doSomething() synchronized too, because you need to lock every time a concurrent access on a mutable part of the class occurs. While you are only reading fooFoo in doSomething, you could at the same time be writing fooFoo in setFooFoo(), thus creating a data race. synchronized essentially causes the function call to take a lock that is associated with the Java-object at entry and to release it once you leave the function.

Alternatively you can use a Lock member inside Foo that you take when you do either. This applies in situations, where you may have multiple independent members that may be accessed safely while the other is being altered. In this case taking two different locks may make your code substantially faster.

For completeness sake it should be mentioned that in some older Java versions (Java 5 only I believe) taking the intrinsic lock of the object through a synchronized method, was substantially slower than using a lock object.

For the deadlock problem: you are right to worry about this, but consider it as a separate issue, after you make your object thread-safe in the first place. The question there is what other locks are taken and it is not possible to answer this with just what you posted. Given your object, synchronizing the read/write on the object can not deadlock by itself, because only one operation can be active at the time, and they do not take any other locks from what you show in your code.

Upvotes: 0

Related Questions