CODEWITHSUNDEEP
javamultithreadingthread-safetysynchronized
Elliott
Elliott

Reputation: 5609

Thread Safe Copying of Objects in Java

I have a static array of classes similar to the following:

public class Entry {
    private String sharedvariable1= "";
    private String sharedvariable2= "";
    private int sharedvariable3= -1;

    private int mutablevariable1 = -1
    private int mutablevariable2 = -2;

    public Entry (String sharedvariable1, 
                  String sharedvariable2, 
                  int sharedvariable3) {
        this.sharedvariable1 = sharedvariable1;
        this.sharedvariable2 = sharedvariable2;
        this.sharedvariable3 = sharedvariable 3;
    }

    public Entry (Entry entry) {  //copy constructor. 
        this (entry.getSharedvariable1, 
              entry.getSharedvariable2, 
              entry.getSharedvaraible3);
    }
....
/* other methods including getters and setters*/
}

At some point in my program I access an instance of this object and make a copy of it using the copy constructor above. I then change the value of the two mutable variables above. This program is running in a multithreaded environment. Please note. ALL VARIABLES ARE SET WITH THEIR INITIAL VALUES PRIOR TO THREADING. Only after the program is threaded an a copy is made, are the variables changed. I believe that it is thread safe because I am only reading the static object, not writing to it (even shared variable3, although an int and mutable is only read) and I am only making changes to the copy of the static object (and the copy is being made within a thread). But, I want to confirm that my thinking is correct here.

Can someone please evaluate what I am doing?

Upvotes: 1

Views: 4697

Answers (5)

Elliott
Elliott

Reputation: 5609

The answer is that it is thread safe under the conditions outlined since I am only reading from the variables in their static state and only changing the copies.

Upvotes: 0

MALfunction84
MALfunction84

Reputation: 500

It is not thread-safe. You need to wrap anything that modifies the sharedvariables thusly:

synchronized (this) {
    this.sharedvariable1 = newValue;
}

For setters, you can do this instead:

public synchronized void setSharedvariable1(String sharedvariable1) {
    this.sharedvariable1 = sharedvariable1;
}

Then in your copy constructor, you'll do similarly:

public Entry (Entry entry) {
    this();
    synchronized(entry) {
        this.setSharedvariable1(entry.getSharedvariable1());
        this.setSharedvariable2(entry.getSharedvariable2());
        this.setSharedvariable3(entry.getSharedvariable3());
    }
}

This ensures that if modifications are being made to an instance, the copy operation will wait until the modifications are done.

Upvotes: 3

bennidi
bennidi

Reputation: 2112

I don't really get, why you consider private instance variables as shared. Usually shared fields are static and not private - I recommend you not to share private instance variables. For thread-safety you should synchronize the operations that mutate the variables values.

You can use the synchronized keyword for that but choose the correct monitor object (I think the entry itself should do). Another alternative is to use some lock implementation from java.util.concurrent. Usually locks offer higher throughput and better granularity (for example multiple parallel reads but only one write at any given time).

Another thing you have to think about is what is called the memory barrier. Have a look at this interesting article http://java.dzone.com/articles/java-memory-model-programer%E2%80%99s

You can enforce the happens before semantic with the volatile keyword. Explicit synchronization (locks or synchonized code) also crosses the memory barrier and enforces happens before semantics.

Finally a general piece of advice: You should avoid shared mutable state at all costs. Synchronization is a pain in the ass (performance and maintenance wise). Bugs that result from incorrect synchronization are incredibly hard to detect. It is better to design for immutability or isolated mutability (e.g. actors).

Upvotes: 0

yair
yair

Reputation: 9245

It's not thread safe. And I mean that is does not guarantee thread safety for multiple threads that use the same Entry instance.

The problem I see here is as follows:

  1. Thread 1 starts constructing an Entry instance. It does not keep that instance hidden from other threads access.
  2. Thread 2 accesses that instance, using its copy constructor, while it is still in the middle of construction.

Considering the initial value for Entry's field private int sharedvariable3= -1;, the result might be that the new "copied" instance created by Thread 2 will have its sharedvariable3 field set to 0 (the default for int class fields in java).

That's the problem.

If it bothers you, you've got to either synchronize the read/write operations, or take care of Entry instances publication. Meaning, don't allow access of other threads to an Entry instance that is in the middle of construction.

Upvotes: 0

Kai
Kai

Reputation: 39632

It is not thread-safe, you should synchronize in your copy constructor. You are reading each of the three variables from the original object in your copy constructor. These operations are not atomic together. So it could be that while you are reading the first value the third value gets changed by another thread. In this case you have a "copied" object in an inconsistent state.

Upvotes: 0

Related Questions