Simple yet threadsafe file cache in java

Question

I'm attempting to cache contents of a file to avoid frequent reads, using a very simple implementation in a single class. The file contains a list of emailids, one per line. The file changes rarely, and the cache should be updated immediately when it does. So I've checked the file's timestamp in each read operation. The class will be used in servlets and rest apis - as member variable. Although I've synchronized the only public method of the class, I'm not sure whether the code is threadsafe. Do I need to add/change something here to ensure thread safety ?

public class CachedFile {

    private File file ;
    private long lastModified ;
    private List<String> fileLines ;
    private boolean fileCached ;

    public CachedFile( final String inputFileName )
    {
            file = new File( inputFileName ) ;
            fileCached = false ;
    }

    public synchronized List<String> getLines() throws IOException
    {
            //if file is not cached or if its modified after the last read, read & cache it
            if( fileCached == false || fileModifiedAfterLastRead() )
            {
                    readFile() ;
            }

            return fileLines ;
    }

    private void readFile() throws IOException
    {
            //read file to cache
            fileLines = FileUtils.readLines( file ) ;

            //cache the last modified time
            lastModified = file.lastModified() ;

            //set the cached flag to true
            fileCached = true ;
    }

    private boolean fileModifiedAfterLastRead()
    {
            return( file.lastModified() > lastModified ) ;
    }
}

Answer 1

Since you mentioned that the file could be written by another process (assuming in another jvm), there is no way you can introduce the Object monitor based thread safety. So you can depend upon the last modified time of the file object. You can also remove the synchronization from your getLines() if you dont return the instance variable. But you do return that so synchronization should be there in getLines().

Basically you want to read the file without the data corruption. That could happen if another process is writing to the file when your process reads from it. If that is the case, you want to re-read the file. Here is how I would implement it.

 private void readFile() throws IOException {
     int retryCount = 5;

     while (retryCount > 0) {

       long beforeRead = file.lastModified();
        //read file to cache
       fileLines = FileUtils.readLines(file);

       if (beforeRead == file.lastModified())
           break;

       retryCount--;
     }
  //cache the last modified time
    lastModified = file.lastModified() ;

  //set the cached flag to true
    fileCached = true ;
}

The while loop will retry the read operation if there is a mismatch. Also the retry has to be finite number to avoid looping forever in case of any file system failures.

Answer 2

Your cache is fine and correctly synchronized.

One thing I would change is to remove the throws IOException from the public method. Just for convenience of the calling code.

For the process that updates the file, make sure that it updates the file atomically. That is, first write the data to a temporary file and then rename that to the final file.