2

I am trying to understand safe publication in case of Effectively Immutable classes. For my class I cannot come up with a scenario in which it would be thread unsafe. Do I need to add some other safe gaurd?

CLARIFICATION: Container elements are thread safe

 public class Container<E> {
    private LinkedList<E> data;

     public Container() {
        this.data = new LinkedList<E>();
    }

    public Container(final Container<E> other) {
        this.data = new LinkedList<E>(other.data);
    }

    public Container<E> add(E e) {
        Container<E> other_cont= new Container<E>(this);
        other_cont.data.add(e);
        return other_cont;
    }

    public Container<E> remove() {
        Container<E> other_cont= new Container<E>(this);
        other_cont.data.remove(0);
        return other_cont;
    }

     public E peek() {
        if(this.data.isEmpty())
            throw new NoSuchElementException("No element to peek at");
        return this.data.get(0);
    }

     public int size() {
        return this.data.size();
    }
 }
Suryavanshi
  • 595
  • 1
  • 7
  • 24
  • If the elements are themselves thread unsafe, there could be issues. Say, for example, you have a Container. You could theoretically call peek() to retrieve the same LinkedList in two separate threads, and then you have a problem. – Vincent May 13 '15 at 02:35
  • For the sake of argument, lets say that the elements are themselves thread safe. – Suryavanshi May 13 '15 at 02:37
  • If add or remove can be called then it is unsafe and concurrent calls could corrupt the doubly linked list. – Ben Manes May 13 '15 at 02:41
  • @Ben But as add and remove are not modifying the list itself, how is it unsafe? – Suryavanshi May 13 '15 at 02:42
  • 1
    i'm sorry, you're correct. I misread that. If you use copy-on-write then its safe – Ben Manes May 13 '15 at 02:47

1 Answers1

1

This looks fine, as long as a Container is not published unsafely (which we should never do anyway).

But, for the sake of discussion, let's say a Container object is passed through unsafe publication

 static Container shared; // not volatile

 // thread 1
 shared = containerX.add( e );

 // thread 2
 shared.peek();

this is not thread safe; thread 2 can observe corrupt state.

To fix that, we'll need final variable; and all writes should finish before the constructor exit. In your code, you modify other_cont after new Container, which is the problem.

Personally, I would do it this way

 public class Container<E> {
    final private LinkedList<E> data;

    public Container() {
        this.data = new LinkedList<E>();
    }

    Container(LinkedList<E> data) {
        this.data = data;
    }

    public Container<E> add(E e) {

        LinkedList<E> copy = new LinkedList<>(data); 
        copy.add( e );
        return new Container<>(copy);
    }

    ... etc

Rule of thumb for designing an immutable class - all fields must be final, and all writes must be done before constructor exit.

ZhongYu
  • 19,446
  • 5
  • 33
  • 61
  • I assume you mean "safely"; Can you elaborate on your answer, are you suggesting some changes in the add remove methods? – Suryavanshi May 13 '15 at 02:40
  • @Suryavanshi oops, I need double-negative on that sentence. – ZhongYu May 13 '15 at 02:44
  • final in `final private LinkedList data;` only ensures that the reference points to the same LinkedList. It won't forbid any further mutation of the list – Suryavanshi May 13 '15 at 02:46
  • only Container class can modify it; and Container class never modifies it. – ZhongYu May 13 '15 at 02:48
  • how come modifying the `other_cont` is a problem?? because IMHO each thread will always have a separate copy of `other_cont` – Suryavanshi May 13 '15 at 02:49
  • "only Container class can modify it; and Container class never modifies it" then why use final at all?? – Suryavanshi May 13 '15 at 02:52
  • that is the intricate part ... I can link to the [spec](http://docs.oracle.com/javase/specs/jls/se8/html/jls-17.html#jls-17.5.1) ... the gist of it is that all writes must be done before constructor exit. – ZhongYu May 13 '15 at 02:52
  • 1
    "why use final at all" - good question. `final` has two separate, unrelated semantics; 1st, it means the field cannot be rewritten; 2nd, it has some semantics for memory model. we don't really need the 1st in this class, but we do need the 2nd. – ZhongYu May 13 '15 at 02:56
  • Point conceded on use of final, still what I don't get is the problem with modifying `other_cont` as its not published until the modification has finished – Suryavanshi May 13 '15 at 02:59
  • It fails if JVM does instruction reordering; however, `final` restrict that reordering. – ZhongYu May 13 '15 at 03:02