CODEWITHSUNDEEP
javaconcurrencythread-safety
Pacerier
Pacerier

Reputation: 89743

Non-final variables and thread safety in JCIP

In JCIP we have a piece of code which looks like this:

Listing 4.2:

@ThreadSafe
public class PersonSet {
    @GuardedBy("this")
    private final Set<Person> mySet = new HashSet<Person>(); // line 3

    public synchronized void addPerson(Person p) {
        mySet.add(p);
    }

    public synchronized boolean containsPerson(Person p) {
        return mySet.contains(p);
    }
}

I was wondering if we change the third line to this:

   private Set<Person> mySet = new HashSet<Person>(); // line 3, removes final

Is it true to say that the class is no longer thread-safe because the non-final variable mySet could have been null, even after the constructor exits and a reference to the PersonSet instance is published?

For example, is it true to say that a calling code like this may fail, or am I misunderstanding somthing? :

PersonSet p = new PersonSet();
SendToThreadB(p);

What if I have a restriction that does not allow the field to be marked "final" (like I may have to swap it with a new instance), what solutions are there to ensure that the class is still thread-safe without using final ?

Upvotes: 1

Views: 470

Answers (4)

Stephen C
Stephen C

Reputation: 719336

If you remove the final, the class is still thread-safe because of the synchronized modifier on the methods. They will provide the happens-before relationship that guarantees that everyone will see the same value for the mySet reference.

The point about the final is that it makes this variant thread-safe with respect to the class ifself.

public class PersonSet {
    private final Set<Person> mySet = new HashSet<Person>();

    public Set<Person> getSet() {
        return mySet;
    }
}

Note that there is no synchronized modifier on the getter.

(Of course, any code that gets hold of the HashSet object needs to synchronize their operations on the object ... somehow ... which means that this is NOT an example of good design.)

Upvotes: 1

Tom Hawtin - tackline
Tom Hawtin - tackline

Reputation: 147154

If you remove the final, instances become unsafe when used after unsafe publication. That is, if another thread gets access to the object without going through synchronized/volatile to produce an appropriate happens-before relationship then it may see a partially initialised object. In this case it would probably fail in a relatively safe way giving a NullPointerException on dereferencing the mySet field. In theory, another thread could see the reference to the HashSet, but some or all of the fields of that object may not have been set.

Upvotes: 2

yshavit
yshavit

Reputation: 43401

I think you're right -- you need the final keyword, and without it, another thread could see mySet as null.

Constructors have no memory barriers or synchronization, other than for final fields. So even though the various modifications to the HashSet are all guarded by the same monitor (the one that belongs to this), the assignment of that object's reference to the mySet field is not guarded.

Upvotes: 0

C. K. Young
C. K. Young

Reputation: 223153

If you want to be able to change the field value, make the field volatile if you want changes to the field to be visible to other threads without having to use other happens-before mechanisms. (Reading and writing a volatile field are happens-before events.)

Of course, if you always use other happens-before systems (e.g., synchronized, mutex locks, BlockingQueue, Exchanger, setting the field value before creating the thread you're sending it to, etc.), then volatile isn't necessary. But that is more fragile, because if you later change code such that no happens-before happens any more, then you've created a bug.

Upvotes: 2

Related Questions