Thread safety of a fluent like class using clone() and non final fields

Question

This fluent like class is not strictly immutable because the fields are not final, but is it thread safe, and why?

The thread safety issue I'm concerned with is not the race condition, but the visibility of the variables. I know there is a workaround using final variables and a constructor instead of clone() + assignment. I just want to know if this example is a viable alternative.

public class IsItSafe implements Cloneable {

    private int foo;
    private int bar;

    public IsItSafe foo(int foo) {
        IsItSafe clone = clone();
        clone.foo = foo;
        return clone;
    }

    public IsItSafe bar(int bar) {
        IsItSafe clone = clone();
        clone.bar = bar;
        return clone;
    }

    public int getFoo() {
        return foo;
    }

    public int getBar() {
        return bar;
    }

    protected IsItSafe clone() {
        try {
            return (IsItSafe) super.clone();
        } catch (CloneNotSupportedException e) {
            throw new Error(e);
        }
    }
}

You can make this slightly safer by making IsItSafe final. Are you worried about someone changing foo or bar with reflection? Why are you concerned about their visibility? — alpian, Mar 08 '12 at 17:59
I'm not worried about someone modifying the field through reflection or by extending the class. I'm asking because I designed a similar class and its thread safety has been questioned. — Emmanuel Bourg, Mar 08 '12 at 18:09
FYI, I have posted a continuation of the question here: http://stackoverflow.com/questions/9633771/how-can-one-break-this-non-thread-safe-object — assylias, Mar 09 '12 at 12:04

score 1 · Answer 1 · edited May 23 '17 at 12:03

1

This thread is fairly unanimous that the class is not thread safe because of visibility problems.

Why do you say that the class is not immutable? The state of the class is defined by foo and bar which, for any specific instances, can't be changed from outside the class once the instance is created. So it is immutable, even if the fields are not explicitly declared final.

The only place where foo and bar are changed (in the foo() and bar() methods), the changes are done on a local variable which is by definition only accessed by one thread at a time.

EDIT

I think this is an example of Stack Confinement as defined in Java Concurrency in Practice (3.3.2), which makes the foo() and bar() methods thread safe because clone is not allowed to escape the method before being fully constructed.

Local variables are intrinsically confined to the executing threead; they exist on the executing thread's stack, which is not accessible to other threads.

edited May 23 '17 at 12:03

Community

1
1

answered Mar 08 '12 at 16:40

assylias

321,522
82
660
783

That's what I think but I have a doubt. If the instance returned by foo() or bar() is shared with another thread, will it see the updated value for bar or foo? – Emmanuel Bourg Mar 08 '12 at 16:48
1

(I am not the downvoter.) Regarding your immutable argument: Have you considered instruction reordering? If what you are saying here was right, there would be no need for the final modifier to create thread-safe immutable objects. Regarding your stack confinement argument: The stack local variable is `clone`. The visibility problem concerns the fields `foo` and `bar`, which obviously are not stack-local. – Joe23 Mar 08 '12 at 19:08
+1 for the time you dedicated to the issue, thank you assylias! – Emmanuel Bourg Mar 09 '12 at 13:48

score 1 · Accepted Answer · edited May 23 '17 at 12:03

1

You are not holding a Lock while setting the field and as you mention yourself the field is not final.

Therefore from a visibility standpoint this approach is not thread-safe.

Some further clarifications here: https://stackoverflow.com/a/9633968/136247

Update regarding the question of using volatile:

For the sake of the argument using volatile fixes the threading issue here.
However you should reconsider final fields and a copy constructor because:

Field access will be slightly faster (the reads can always come from the cpu cache)
You avoid the discouraged use of clone (see Effective Java by Josh Bloch)
Constructor with final fields is a known idiom for immutable classes and will be easily recognised by readers of the code
Marking fields volatile while intending for them to be immutable is a contradiction in and of itself ;)

edited May 23 '17 at 12:03

Community

1
1

answered Mar 08 '12 at 19:01

Joe23

5,683
3
25
23

I am not sure what you mean by *logical*. Do you mean *practical*? Or do you mean if there was no Java Memory Model and this was pseudo code? – Joe23 Mar 09 '12 at 12:23
So marking the fields as volatile would fix the visibility issue, right? – Emmanuel Bourg Mar 09 '12 at 13:49
@EmmanuelBourg According to the link shared by Joe23, yes: http://www.cs.umd.edu/~pugh/java/memoryModel/jsr-133-faq.html#volatile – assylias Mar 09 '12 at 13:58
Answer accepted, thank you for the detailed information in the related question Joe, that's exactly what I was looking for. – Emmanuel Bourg Mar 09 '12 at 14:38

Thread safety of a fluent like class using clone() and non final fields

2 Answers2

Linked