Thread safety of Java example using WeakReference

I am reading up on weak references in Java after seing a SO post and realising I didn't really know what they were.

The following code is from pp. 457, Chapter 17: "Garbage Collection and Memory" in "The Java Programming Language, Fourth Edition" by Arnold, Gosling and Holmes

import java.lang.ref.*;
import java.io.File;

class DataHandler {
    private File lastFile;        // last file read
    private WeakReference<byte[]> 
                         lastData;// last data (maybe)

    byte[] readFile(File file) {
        byte[] data;

        // check to see if we remember the data
        if file.equals(lastFile) {
            data = lastData.get();
            if (data != null)
                return data;
        }

        // don't remember it, read it in
        data = readBytesFromFile(file);
        lastFile = file;
        lastData= new WeakReference<byte[]>(data);
        return data;
    }
}

I am trying to understand, just for the exercise of it, if this code is thr开发者_StackOverflow社区ead safe, with the part of code I am focusing on being the lines

data = lastData.get();
if (data != null)
    return data;

My thinking is as follows: "data" is thread confined and is set to reference the referent of the "lastData" WeakReference. This creates a strong reference to the referent, hence even if after the null check, all other strong references to the referent outside readFile's scope disappears (what's the correct term ?) and even assuming the referent is not softly reachable, the garbage collector is not allowed to clear the weak reference thereby making the referent finalizable, because there still is the local strong reference to it from data. Hence if data is not null in the data != null line, it can not be null when returned in the following line. Correct ?

I think the example code is not thread safe, but for different reasons than using the weak reference:

The usage of the weak reference is fine, exactly for the reasoning you pointed out: The code creates a strong reference which is held in the data variable. Thus, the GC can't collect the bytes and thus also the WeakReference will stay intact; so using this code in a single threaded application should be safe. The problems come with multiple threads:

The access to the file and lastData fields is not synchronized, thus there is no guarantee that two Threads working with the readFile(..) method interact at all (which would be the unlikely "best" case). It is important to note that these fields must be accessed atomically, and if there is no other place where they are touched, the easiest fix would be to declare the readFile method synchronized. This would hurt performance rather badly, because the file reading would then happen inside the synchonized block, possibly causing bad contention.

Once a reference has been assigned to the local data variable, the object is not eligible for garbage collection. It is strongly reachable because there is a reference to it in the thread's stack.

Technically speaking its not thread safe because the lastData and lastFile are not volatile. It is possible for a second thread to see an old copy of these references. It may not make much difference to your application.