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 ) ;
 }
}

• Tiny Rick: who will write the file ?? You should synchronize the methods responsible for reading and writing if you want thread-safety...
• amolkul: The file will be changed by a different application. Currently its not possible to get the reading / writing to synchronize.
• Andrew S: The other process which updates the file - is it possible the other process could completely replace the file? If yes, file might no longer be valid.
• amolkul: @AndrewS That seems to be valid point. I will create a new 'file' object each time I use it. Since I only need to cache the lastmodified time and the lines from the file, that should work.

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.

• amolkul: Thanks Tiny Rick. But if I don't synchronize, will the list fileLines have consistent data ?
• Tiny Rick: Thats what we made sure in the while loop. We checked if the data has been changed at the time our read operation by once again checking the modified time with our pre-read modified time. So answering your question.. Yes without synchronization your list of file lines will be consistent with the file.
• Imus: The retry will never work like it is now though. Update the beforeRead right before retryCount--; That way you check if it has stayed the same in that retry attempt.
• Tiny Rick: Thanks @Imus . Now beforeRead will be initialized just before the actual read operation.
• Roland Illig: The synchronized is still necessary. Otherwise one thread might see incomplete lines in the list. Read the whole Java Memory Model documentation for more information.

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.

• 200_success: How else would you handle IO errors?
• Roland Illig: In the constructor, load the file or fail (depending on the application needs). Then, after the initial loading has succeeded, if loading the file should ever fail, just log that and keep the previous list. Add some monitoring for the logging. Rationale: since there are probably several places in the code where the cache is used, it would not be correct to do the error handling there.
• 200_success: If reading fails, writing to a log file might also fail. The bigger problem, though, is that the rest of the consumer application won't be able to report the error; it has to learn of the staleness of the data in a roundabout way. That is a much more onerous design than just propagating an IOException.
• amolkul: @RolandIllig +1 for mentioning atomic updates