0

I've written similar code to this in one of my applications but I'm not sure whether it is thread safe.

public class MyClass {
    private MyObject myObject = new MyObject();

    public void setObject(MyObject o) {
        myObject = o;
    }

    public MyObject getObject() {
        return myObject;
    }
}

The setObject() and getObject() methods will be called by different threads. The getObject() method is going to be called by a thread that keeps drawing on a Canvas. For optimum FPS and smooth motion, I don't want that thread to be kept waiting for a synchronization lock. Hence, I want to avoid using synchronization unless it is really necessary. So is it really necessary here? Or is there any other better way to solve this problem?

And by the way, it doesn't matter if the thread receives an older copy of the object.

Suyash
  • 2,531
  • 2
  • 17
  • 29
  • "I want to avoid using synchronization unless it is really necessary" - did you evaluate synchronization overhead? You redraw canvas, say, 30 times per second, so once in 33 milliseconds you call getObject(). Synchronization (just lock, without wait()) takes less than a microsecond. – Alexei Kaigorodov Feb 14 '14 at 11:26
  • Yes thank you for noting that. I didn't actually evaluate how much time will be compromised. But I'm going to use `volatile` instead of synchronization as that will be enough in this case seeing all the other replies. – Suyash Feb 14 '14 at 11:41

5 Answers5

5

As for the status of your current version, it is definitely not thread-safe because concurrent access of myObject will establish a data race.

You didn't specify this, but if MyObject is not thread-safe itsef, then your program will not be thread-safe, regardless of what you do to the code you have shown.

it doesn't matter if the thread receives an older copy of the object.

The Java Memory Model allows much worse things than that to happen to objects accessed via a data race:

  • a thread may receive always the same object (the first one it happened to read);
  • a thread may observe the object with only some of the reachable values initialized (a torn object).

For optimum FPS and smooth motion, I don't want that thread to be kept waiting for a synchronization lock.

Have you spent any effort to actually measure how much time your threads are waiting for the lock? My guess: you didn't because that time is so short as to be undetectable.

However, your case doesn't even call for locks: just making your instance variable volatile will be enough to guarantee safe sharing of the object between threads.

Marko Topolnik
  • 195,646
  • 29
  • 319
  • 436
  • Thank you very much for your help! I will just make the variable volatile as you mentioned. – Suyash Feb 14 '14 at 11:33
4

No it's not thread safe - this could happen:

  • a thread may not see the latest version of myObject when calling getObject (which you can live with apparently)
  • a thread may never see any updates made to myObject by other threads when calling getObject
  • a thread may see an updated reference to a MyObject that is in a inconsistent state (partially constructed for example)

The easiest way to solve these issues is to mark myObject volatile.

assylias
  • 321,522
  • 82
  • 660
  • 783
  • How would a thread ever see `myObject` has a partially constructed instance? The assignment is done *after* the object is instantiated, and the assignment itself is atomic, is it not? – dcastro Feb 14 '14 at 11:26
  • @dcastro nop - imagine a thread does: `MyObject o = new MyObject(); myClass.setObject(o);` the first statement includes: allocating memory, assigning a reference to `o`, constructing the object. These 3 operations and the following statement can in theory *appear* in a different order when seen from another thread, e.g. `setObject` being called before the object is constructed. Unless you introduce a proper happens-before relationship between the two threads that is. – assylias Feb 14 '14 at 11:29
  • Ah, that's a good point. So a release-fence would be needed before publishing the reference. I wasn't aware the JMM allowed such reordering. – dcastro Feb 14 '14 at 11:36
2

You actually have a number of complications here:

  1. myObject needs to be volatile. (Otherwise other threads may NEVER see changes).

  2. The initial value of myObject will be fully constructed before MyClass is accessed so that is safe in this case, however in general you need to be careful about combining construction of objects and multi threading.

Tim B
  • 40,716
  • 16
  • 83
  • 128
  • 1
    `The initial value of myObject will be fully constructed before MyClass is accessed`---this isn't true if `myObject` is not final. It will *become* true, though, if the plans for a revised Java Memory Model in Java 9 pan out. – Marko Topolnik Feb 14 '14 at 11:22
  • 2. Wrong. You cannot call getObject when instance is not created yet. – m0skit0 Feb 14 '14 at 11:22
  • If an instance of MyClass is published through a data race `myObject` may be seen in a partially constructed state. – assylias Feb 14 '14 at 11:22
  • +1 Without `volatile` the JIT can *inline* a value which means the field is never read again. – Peter Lawrey Feb 14 '14 at 11:22
  • Yes, that's a datarace on MyClass - not a datarace on myObject though. – Tim B Feb 14 '14 at 11:22
  • Non final and non volatile fields not guaranteed to be visible to other threads at the end of the constructor. – Peter Lawrey Feb 14 '14 at 11:24
0

Yes, it is thread-safe under the said conditions, so long as you mark myObject as volatile. You will always get a correct MyObject instance from getObject().

Tim B
  • 40,716
  • 16
  • 83
  • 128
m0skit0
  • 25,268
  • 11
  • 79
  • 127
0

You should make the shared variable volatile, to let a thread know that other threads/processes/etc may change its value.

Beside that, there's no concurrency issue in your code.

The second line, which creates an instance of MyObject when an instance of MyClass is created, is perfectly fine. No one will have access to the shared variable until the instance of MyObject is fully constructed (unless you leak the shared variable from within the constructor).

The setObject method is also fine - all it does is assign an object to the shared variable myObject. And since assignments are atomic, there's nothing to worry about.

dcastro
  • 66,540
  • 21
  • 145
  • 155