0

I've read Oracle's documentation, which states (among a few other things) that:

Reads and writes are atomic for reference variables

So that means, assuming I understand this correctly, that the below code is thread safe without needing volatile, synchronized, or using a Lock class, since the actual assignment of otherHashlist to hashlist is atomic and the assignment from hashlist to tempHashlist is also atomic.

public class SomeClass
{
  private HashMap<Integer, ArrayList<MyObject>> hashlist = null;

  // ...Thread/Runnable logic to periodically call set()...

  private void set()
  {
    HashMap<Integer, ArrayList<MyObject>> otherHashlist = new HashMap<Integer, ArrayList<MyObject>>();

    // ...populate otherHashlist...

    hashlist = otherHashlist;
  }

  public ArrayList<MyObject> get(int i)
  {
    HashMap<Integer, ArrayList<MyObject>> tempHashlist = hashlist;

    return new ArrayList<MyObject>(tempHashlist.get(i));
  }
}

Additionally, hashlist is never accessed in any way outside of get() and set(). set() is also not able to be called, directly or indirectly, by anything outside of the class. The ArrayList returned by get() is new, so operations that modify the ArrayList (set(), remove(), etc) will not affect the original ArrayList in hashlist. I also have not defined any setter methods on MyObject, all of whose member variables are either private final int, private final long, or private final String.

So, my question then, is: Is this code actually thread safe? Or is there some assumption I'm making/angle I'm missing that would make this unsafe?

Pentarctagon
  • 413
  • 2
  • 5
  • 12
  • 1
    Unrelated: why do you need the following line: `HashMap> tempHashlist = hashlist;` when you can get directly from `hashlist` ? – Nir Alfasi Aug 19 '17 at 17:19
  • Assigning all the elements of `hashlist` to a new ArrayList is definitely not atomic, and I wasn't sure what would happen if `set()` was called part way through constructing that new ArrayList. – Pentarctagon Aug 19 '17 at 17:54
  • The line I was referring to does nothing like assigning all the elements of hashlist to a new ArrayList - it's a reference assignment. – Nir Alfasi Aug 19 '17 at 17:57

2 Answers2

1

Depends on what is the expected behavior...

If every thread has its own instance of SomeClass - it's thread-safe. So let's assume multiple threads have the same instance:

Now say that two threads call set simultaneously, and right after one of them executes the assignment: hashlist = otherHashlist; the other thread is doing the exact same thing (with different content probably). This means that two consecutive calls to get(i) might return different results, which means consistency is not being kept. Further, since threads have local-caching, it could be that some threads will see an older (stale) copy of hashlist.

Is this an accepted behavior ? if yes (which is a bit weird IMO), then you're good.

I highly recommend reading: Java Concurrency in Practice by Brian Goetz, around chapter: "3.5. Safe Publication"

Nir Alfasi
  • 53,191
  • 11
  • 86
  • 129
  • There will only be one instance(a singleton), but `set()` is only ever called internally - other threads have no way of calling `set()` and there are no public methods that can result in `set()` being called. – Pentarctagon Aug 19 '17 at 17:40
  • @Pentarctagon if `set` is called only once and it's complete *before* any of the reading threads starts reading then you should be good. Otherwise, the fact that `set` is called only internally doesn't matter because other threads might see stale values. – Nir Alfasi Aug 19 '17 at 17:45
  • I'm not sure I understand - how would the reading threads see stale values, if `get()` always first refers to `SomeClass`'s current `hashlist` variable? – Pentarctagon Aug 19 '17 at 17:51
  • If the threads have a copy of the singleton *before* it finished running `set` it is considered as unsafe publication. I highly recommend reading: "Java Concurrency in Practice" by Brian Goetz, around chapter: "3.5. Safe Publication" – Nir Alfasi Aug 19 '17 at 17:53
1

This isn't thread safe, because although setting the field is safe and won't cause overlapping updates, the changes may not actually be visible by other threads. That is, even if Thread1 sets hashlist to something, Thread2 may not see that change. This is because the JVM is allowed to optimize accesses to hashlist in Thread2, say copying the reference into a register so it doesn't need to do a getfield multiple times. In this case, when Thread1 changes hashlist, Thread2 can't see it, because it is no longer using the field, it is using a local copy of it, which it thinks is the field. This question shows what can happen in cases like this.

The easiest thing to do is to label hashlist volatile, which means changes in one thread are actually seen by others.

HTNW
  • 27,182
  • 1
  • 32
  • 60
  • `hashlist` never leaves `SomeClass`, the only thing ever returned is an `ArrayList`. – Pentarctagon Aug 19 '17 at 17:48
  • Doesn't matter; if a `SomeClass` is shared between threads and access to `hashlist` is unsynchronized (`volatile` is a form of synchronization), the problem persists. – HTNW Aug 19 '17 at 17:56
  • @HTNW sorry for being pedantic: volatile is *not* a form of synchronization, volatile mechanism makes sure modifications of a field are visible to all threads, it's a good solution if only one thread updates a field and multiple threads reads it, but it's not a synchronization mechanism, synchronization is stronger since it's supports writes from multiple threads. – Nir Alfasi Aug 19 '17 at 18:46
  • If we're being pedantic, I'll correct my statement and say that there needs to be some *happens-before* relation between writes and reads to `hashlist`, which is provided by `volatile`. – HTNW Aug 19 '17 at 20:45