9

Let's say I have the following,

public class Foo{
    private String bar;

    public String getBar(){
        return bar;
    }

    public void setBar(String bar){
        this.bar = bar;
    }
}

Are these methods automatically threadsafe due to the immutable nature of the String class, or is some locking mechanism required?

mre
  • 43,520
  • 33
  • 120
  • 170

4 Answers4

20

No, this is not threadsafe. Foo is mutable, so if you want to ensure that different threads see the same value of bar – that is, consistency – either:

  • Make bar volatile, or
  • Make the methods synchronized, or
  • Use an AtomicReference<String>.

The reads and writes of bar are themselves atomic, but atomicity is not thread safety.

http://docs.oracle.com/javase/tutorial/essential/concurrency/atomic.html

For in-depth coverage of Java concurrency, grab a copy of Java Concurrency in Practice (aka JCIP).

Matt Ball
  • 354,903
  • 100
  • 647
  • 710
  • 4
    +1 Or simply declare `bar` as `final`, so the reference cannot be re-assigned to another value. Of course, there will be no setter then :) – Eng.Fouad Feb 25 '13 at 17:25
  • 1
    That's immutable. No need for a setter in that case. And you have to be sure to supply a constructor to initialize the value, because you can't change it once you exit the constructor in that case. – duffymo Feb 25 '13 at 17:41
  • ... Out of curiosity, do either of those actually matter? I mean, if your actual problem is a race condition, that can still happen, despite the `volatile` or `synchronized` - it just decreases the 'window' in which it can happen, right? Or am I missing something here? – Clockwork-Muse Feb 25 '13 at 18:09
  • @Clockwork-Muse and what race condition would that be? TBH, questions like these are why I avoid sharing mutable (non-concurrent-library) objects across threads as much as possible. – Matt Ball Feb 25 '13 at 18:11
  • Thread one is updating `bar` continuously. Thread two is waiting for `bar` to read as some value (just spinning with a `while(...)` loop, say). The race condition is for thread two to read the value of `bar` before thread one updates it again. While either of those options make it so that all threads are guaranteed to have the same internal reference (although references are atomic _regardless_), that doesn't matter if they're working on the object at different time-slices. And yeah, stuff like this is why I prefer immutable objects. – Clockwork-Muse Feb 25 '13 at 18:43
  • @Matt Ball if instead String field was mutable, then volatile was not make class thread safe? – gstackoverflow Feb 01 '17 at 21:27
7

You're setting references, and as such String's immutability doesn't come into play. You're not affecting the contents of String.

Brian Agnew
  • 268,207
  • 37
  • 334
  • 440
5

No, not safe.

This is Foo mutable behavior; String's immutability does not accrue to Foo.

public class Foo{
    private String bar;

    public synchronized String getBar(){
        return bar;
    }

    public synchronized void setBar(String bar){
        this.bar = bar;
    }
}
duffymo
  • 305,152
  • 44
  • 369
  • 561
  • +1 What about synchronizing `getBar()`? Maybe some thread setting a new value to `bar`, while the other threads reading `bar`. – Eng.Fouad Feb 25 '13 at 17:30
  • Sorry, you're making no sense to me. I think your comments are only confusing the issue. – duffymo Feb 25 '13 at 17:46
  • @duffymo does a non-synchronized getter with a synchronized setter guarantee consistency across threads? – Matt Ball Feb 25 '13 at 17:50
  • 1
    @duffymo as you have it now, the getter needs to be synchronized to ensure that `bar`'s current state is visible to all threads – Steve Kuo Feb 26 '13 at 07:26
3

No, it's not thread safe.

While String is immutable, the issue comes from the field of Foo. To make this more apparent, consider for example a method whose job would be to append (rather than replace) the value of bar. When it's called from multiple threads, some writes could be lost. The same (lost writes) can happen with your simple setter too, even if it's not obvious initially in this case.

Theodoros Chatzigiannakis
  • 28,773
  • 8
  • 68
  • 104