mibollma is not wrong in his response regarding Android LruCache Thread Safety. People often mistake thread safety and atomicity.
If a class is thread safe, it means that, when for instance two threads call an operation on it, the internals do not break. Vector is such a class with every operation being synchronized. If two different threads call Vector.add, they will both synchronize on the instance and the state is not broken. For instance something like this:
synchronized void add(final T obj) {
objects[index++] = obj;
}
This thread-safe in the sense that no two threads will add an element at the same position. If it would not be synchronized they could both read index = 0 and try to write at that position.
Now why do you still need to synchronize? Imagine you have a case like this:
if(!collection.contains(element)) {
collection.add(element);
}
In that case your operation is not atomic. You synchronize once, when you ask if the element is already present and a second time when you add that element. But there is a window in between those two calls when another thread could make progress and your assumption of the collection not containing the element is broken.
In pseudo code:
if(!coll.contains(element)) { // << you have the exclusive lock here
//Thread 2 calls coll.add(element) << you do not have the lock anymore
coll.add(element); // << doomed!
}
So this is why the answer is correct in the sense that you should synchronize around non-atomic operations like
synchronized(coll) {
if(!coll.contains(element)) { // << you have the exclusive lock here
// Thread 2 wants to call << still holding the lock
// coll.add(element) but
// cannot because you hold the lock
coll.add(element); // << unicorns!
}
}
Because synchronization is pretty expensive the concurrent collections come with atomic operations like putIfAbsent.
Now back to your original question: should you make the LruCache volatile? In general you do not mark the LruCache itself volatile but the reference to it. If such a reference is shared across threads and you plan to update that field then yes.
If a field is not marked volatile a thread might not see the updated value. But again: this is just the reference to the LruCache itself and has nothing to do directly with its contents.
In your specific scenario I would rather have the reference final instead of volatile since no thread should set the reference to null anyways.
The question if you need to put synchronized around your cache operations depends on the case. If you want to create a single atomic operation, like putIfAbsent, then yes.
public void putIfAbsent(final K key, final V value) {
synchronized(lruCache) {
if(!lruCache.containsKey(key)) {
lruCache.put(key, value);
}
}
}
But later in your code, when you call just lruCache.get(key) there is no need to wrap that into a synchronized block itself. Only when you plan to create an atomic operation that should not interfere with another thread.
