Maintaining a temporary directory

Question

I have this class/service, which I need to maintain. Its purpose is to manage a temporary directory so that

1. all content can be deleted on next restart

2. files are not left behind when you're done with a unit of work

Which may sound nice, until you realize that it

1. can be done just by deleting the whole temp dir
2. can be done simply by grouping the unit of work's temp file under a single subdirectory and delete that when you're done - which is what the service does

package com.....platform.documents.services.file;
// ...
import java.io.File;
import java.io.IOException;
import java.nio.file.FileVisitResult;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.nio.file.SimpleFileVisitor;
import java.nio.file.attribute.BasicFileAttributes;
import java.util.Collection;
import java.util.Collections;
import java.util.concurrent.atomic.AtomicLong;
import javax.validation.constraints.NotNull;
import lombok.extern.slf4j.Slf4j;
import org.mapdb.DB;
import org.mapdb.DBException;
import org.mapdb.DBMaker;
import org.mapdb.HTreeMap;
import org.mapdb.Serializer;
/**
 * A manager for a temporary folder with a persistence.
 *
 * TODO: Should be removed, put to a library and refactored.
 * See VOLTRON-3462, VOLTRON-3463, VOLTRON-3464.
 */
@Slf4j
public class FileService {
 private DB mapDB;
 private HTreeMap<String, FileDescriptor> fileCache;
 private HTreeMap<String, FileDescriptor> fileDeleteCache;
 private AtomicLong statsFileCreated = new AtomicLong(0);
 private AtomicLong statsFileDeleted = new AtomicLong(0);
 private AtomicLong statsFileLoaded = new AtomicLong(0);
 private StorageConfiguration storageConfiguration;
 private GeneralConfiguration generalConfiguration;
 private boolean initialized = false;
 private FileService(StorageConfiguration storageConfiguration, GeneralConfiguration generalConfiguration) {
 this.storageConfiguration = storageConfiguration;
 this.generalConfiguration = generalConfiguration;
 }
 public static FileService create(StorageConfiguration storageConfiguration, GeneralConfiguration generalConfiguration) {
 return new FileService(storageConfiguration, generalConfiguration).initialize();
 }
 /**
 * Returns the service's base path plus the given file name.
 */
 public File getFullPath(String fileName) {
 return new File(storageConfiguration.getLocalStorage().getStoragePath().toFile(), fileName);
 }
 /**
 * @param fileName Simple file or directory name.
 * @param isFolder Indicates that the file name represents a folder.
 * @return Pre-existing file or directory, or a newly created one if it didn't exist yet.
 */
 public File getOrCreateFile(String fileName, boolean isFolder) {
 synchronized (fileName.intern()) {
 File file = getFullPath(fileName);
 FileDescriptor fileTracker = this.fileCache.get(fileName);
 if (fileTracker != null && file.exists()) {
 log.debug("Returning existing {} {}", getFsNodeTypeString(fileTracker.getFile()), fileName);
 return fileTracker.getFile();
 }
 this.fileCache.put(fileName, new FileDescriptor(file));
 mapDB.commit();
 this.statsFileCreated.incrementAndGet();
 if (isFolder) {
 boolean result = file.mkdirs();
 if (!result && !file.exists()) {
 throw new ServiceException(ErrorMessages.FS_MK_ERROR, null, log, file.getAbsolutePath());
 }
 }
 log.info("Created {} {}", getFsNodeTypeString(file), fileName);
 return file;
 }
 }
 /**
 * @param fileName Simple file name
 * @return Pre-existing file, or a newly created one if it didn't exist yet.
 */
 public File getOrCreateFile(String fileName) {
 return getOrCreateFile(fileName, false);
 }
 /**
 * Create and manage folder
 *
 * @param folderName Simple folder name
 * @return The File instance representing folder
 */
 public File getOrCreateFolder(String folderName) {
 return getOrCreateFile(folderName, true);
 }
 /**
 * Build folder path from segments and return folder, create it if necessary.
 */
 public File getOrCreateFolder(@NotNull String... segments) {
 return getOrCreateFolder(String.join("/", segments));
 }
 public void deleteFile(File fileToDelete) {
 // TODO: lookup for all content in folder in file cache first and delete them (no need to wait for observer)
 if (fileToDelete == null) {
 return;
 }
 // FIXME: This is a potential bug. The path is not canonicalized .
 final String filePath = getRelativePath(fileToDelete);
 // FIXME: This is a misuse of intern() and may lead to performance problems.
 // Imagine if every library synchronized on interned strings and the path would be simple short strings.
 // We can synchronize on objects in a Set.
 // But worse - if any of the calls inside got stuck, the whole thread would get blocked, and soon enough,
 // we could run out of the fixed tread pool capacity.
 synchronized (filePath.intern()) {
 if (fileDeleteCache.containsKey(filePath)) {
 log.warn("File is already in the retry MapDB cache, skipping: {}", filePath);
 return;
 }
 if (log.isTraceEnabled()) {
 // Log the stack trace correctly.
 Exception ex = new Exception("stacktrace:") {
 public String toString() {
 return getMessage();
 }
 };
 log.trace("Deleting {} {}", getFsNodeTypeString(fileToDelete), filePath, ex);
 } else {
 log.info("Deleting {} {}", getFsNodeTypeString(fileToDelete), filePath);
 }
 // Deleting this file has not been requested yet.
 FileDescriptor fileTracker = this.fileCache.remove(filePath);
 File file2;
 if (fileTracker == null) {
 // We don't have information about the file in cache, however still valid request, so we will try to find and delete the file.
 // This is probably a second try to delete the same file
 log.debug("Request came to delete {} {} not tracked in cache: {}", getFsNodeTypeString(fileToDelete), filePath, this.fileCache);
 file2 = getFullPath(filePath);
 } else {
 file2 = fileTracker.getFile();
 }
 if (file2.exists()) {
 if (fileTracker == null) {
 // fd does not exists, file exists
 fileTracker = new FileDescriptor(file2);
 }
 try {
 doDeleteFile(file2);
 } catch (IOException e) {
 log.error("IOException when trying to delete {} {} . {}", getFsNodeTypeString(file2), file2.getPath(), e.getMessage());
 fileTracker = FileDescriptor.updateCounter(fileTracker);
 fileDeleteCache.put(filePath, fileTracker);
 }
 } else if (fileTracker != null) {
 // fd exists, file does not exists
 statsFileDeleted.incrementAndGet();
 }
 mapDB.commit();
 }
 }
 private void doDeleteFile(File fileToDelete) throws IOException {
 Path path = fileToDelete.toPath();
 if (!path.startsWith(Paths.get(storageConfiguration.getLocalStorage().getStoragePath().toUri()))) {
 throw new IOException("Trying to delete file out of " + storageConfiguration.getLocalStorage().getStoragePath());
 }
 Files.walkFileTree(path, new SimpleFileVisitor<Path>() {
 @Override
 public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException {
 Files.delete(file);
 return FileVisitResult.CONTINUE;
 }
 @Override
 public FileVisitResult visitFileFailed(Path file, IOException exc) throws IOException {
 log.error("Cannot process path {} - {}", file.toString(), exc.getMessage());
 throw exc;
 }
 @Override
 public FileVisitResult postVisitDirectory(Path dir, IOException exc) throws IOException {
 Files.delete(dir);
 return FileVisitResult.CONTINUE;
 }
 });
 this.statsFileDeleted.incrementAndGet();
 }
 /**
 * Supposed to be called exclusively by {@link com....platform.documents.jobs.FileServiceObserver}.
 */
 public void retryDeleteFile(File fileToDelete) {
 final String filename = getRelativePath(fileToDelete);
 log.info("{} {} '{}'", fileToDelete.exists() ? "Deleting" : "Skipping non-existent", getFsNodeTypeString(fileToDelete), filename);
 synchronized (filename.intern()) {
 FileDescriptor fileTracker = fileDeleteCache.get(filename);
 if (fileTracker == null) {
 fileDeleteCache.remove(filename);
 mapDB.commit();
 return;
 }
 File f = fileTracker.getFile();
 if (f.exists()) {
 try {
 doDeleteFile(f);
 fileDeleteCache.remove(filename);
 } catch (Exception ex) {
 log.error("{} when re-trying to delete {} '{}': {}", ex.getClass().getSimpleName(), getFsNodeTypeString(f), f.getPath(), ex.getMessage());
 fileTracker = FileDescriptor.updateCounter(fileTracker);
 fileDeleteCache.put(filename, fileTracker);
 }
 } else {
 fileDeleteCache.remove(filename);
 statsFileDeleted.incrementAndGet();
 }
 mapDB.commit();
 }
 }
 /**
 * Updates the "file descriptor's" (not a real file descriptor) modified time.
 *
 * FIXME: This is not used anywhere except tests. Remove or VOLTRON-3462.
 */
 public void update(File fileToUpdate) {
 if (fileToUpdate == null)
 return;
 final String filename = fileToUpdate.getName();
 synchronized (filename.intern()) {
 FileDescriptor fileTracker = this.fileCache.get(filename);
 if (fileTracker != null) {
 log.debug("Update {} {}", getFsNodeTypeString(fileToUpdate), filename);
 fileTracker = FileDescriptor.updateModified(fileTracker);
 this.fileCache.put(filename, fileTracker);
 mapDB.commit();
 }
 }
 }
 public String getTemporaryLocation() {
 return storageConfiguration.getLocalStorage().getStoragePath().toString();
 }
 public Collection<FileDescriptor> getInUseFiles() {
 return Collections.unmodifiableCollection(this.fileCache == null ? Collections.emptyList() : this.fileCache.getValues());
 }
 public Collection<FileDescriptor> getToDeleteFiles() {
 return Collections.unmodifiableCollection(fileDeleteCache == null ? Collections.emptyList() : fileDeleteCache.getValues());
 }
 public long getStatsFileCreated() {
 return this.statsFileCreated.longValue();
 }
 public long getStatsFileDeleted() {
 return this.statsFileDeleted.longValue();
 }
 /**
 * Create a directory (with all parents) if does not exist.
 *
 * @throws IOException if the directory could not be created
 */
 private static void checkLocation(Path location) throws IOException {
 if (location != null && !Files.isDirectory(location)) {
 Files.createDirectories(location);
 }
 }
 /**
 * @param mapDbFile throws InitializeServerException if the directory could not be created
 */
 private static void checkMapDbLocation(Path mapDbFile) {
 Path mapDbDir = mapDbFile.getParent();
 try {
 checkLocation(mapDbDir);
 } catch (IOException e) {
 throw new ServiceException(ErrorMessages.FS_INIT_MAPDB, e, log, mapDbDir);
 }
 }
 /**
 * @param storageLocation throws InitializeServerException if the directory could not be created
 */
 private static void checkStorageLocation(Path storageLocation) {
 try {
 checkLocation(storageLocation);
 } catch (IOException e) {
 throw new ServiceException(ErrorMessages.FS_INIT_STORAGE, e, log, storageLocation);
 }
 }
 /**
 * Return relative path to configured local storage
 */
 private String getRelativePath(File file) {
 return this.storageConfiguration.getLocalStorage().getStoragePath().relativize(file.toPath()).toString();
 }
 /**
 * @return description of given File object for logging purposes. ("file"/"folder"/"non-existent path"/"path").
 */
 private String getFsNodeTypeString(File file) {
 if (!file.exists())
 return "non-existent path";
 if (file.isDirectory())
 return "directory";
 if (file.isFile())
 return "file";
 return "path";
 }
 public long getStatsFileLoaded() {
 return this.statsFileLoaded.longValue();
 }
 /**
 * Cache initialization
 */
 private FileService initialize() {
 if (this.initialized) {
 log.warn("Reinitialization attempt!");
 return this;
 }
 final Path fileServiceCacheLocation = this.generalConfiguration.getMapDBLocation().resolve("fileService");
 checkStorageLocation(this.storageConfiguration.getLocalStorage().getStoragePath());
 checkMapDbLocation(fileServiceCacheLocation);
 try {
 if (mapDB == null) {
 log.info("Init FileService DB...");
 mapDB = DBMaker
 .fileDB(fileServiceCacheLocation.toFile())
 .transactionEnable()
 .closeOnJvmShutdown()
 .make();
 }
 if (this.fileCache == null || this.fileCache.isClosed()) {
 this.fileCache = mapDB.hashMap("fileCache")
 .keySerializer(Serializer.STRING)
 .valueSerializer(Serializer.JAVA)
 .createOrOpen();
 }
 log.info("Cache size: {}", this.fileCache.size());
 if (fileDeleteCache == null || fileDeleteCache.isClosed()) {
 fileDeleteCache = mapDB.hashMap("fileDeleteCache")
 .keySerializer(Serializer.STRING)
 .valueSerializer(Serializer.JAVA)
 .createOrOpen();
 log.info("Delete cache size: {}", fileDeleteCache.size());
 fileDeleteCache.putAll(this.fileCache);
 statsFileLoaded.set(fileDeleteCache.size());
 this.fileCache.clear();
 mapDB.commit();
 }
 mapDB.getStore().compact();
 this.initialized = true;
 } catch (DBException e) {
 throw new ServiceException(ErrorMessages.FS_INIT_MAPDB, e, log, this.generalConfiguration.getMapDBLocation());
 }
 return this;
 }
 /**
 * For junit testing.
 */
 public void shutdown() {
 this.fileCache.close();
 this.fileCache = null;
 fileDeleteCache.close();
 fileDeleteCache = null;
 mapDB.close();
 mapDB = null;
 statsFileDeleted.set(0);
 statsFileCreated.set(0);
 statsFileLoaded.set(0);
 }
}

• hoffmale: Just a note: Publishing code of your employer might get you into trouble if you don't have permission. I don't know you or your situation, so I can only assume you do have permission, but be aware of this possible issue when presenting the results.
• Raystafarian: Please add a brief description of what the code does as the title - generic titles won't entice people to answer

Answer 1

Thanks for sharing your code.

know the language

You are doing a lot of effort to duplicate functionality Java gives you for free.

When creating a File object you can call .deleteOnExit() on it an java will do the nasty stuff. Your service only needs to handle "recreation" of a file and explicit deletes.

possible bug

In your method

File getOrCreateFile(String fileName, boolean isFolder)

the statement

 if (isFolder) {

does not have an else branch. This means your service only creates folders, no regular files.

Tell! Don't aks! (or the power of enums)

The (possible) bug could have ben easily avoided (or at least become more obvious) by applying the Tell, don't ask! principle like this:

• create an enum providing the code currently in the body of your if statement which is specific to folders:

  enum FileOrDir {
   DIR{
   @Override
   boolean create(File file){
   return result = file.mkdirs();
   }
   };
   abstract boolean create(File file);
  }
  

• Add another constant for regular files:

  enum FileOrDir {
   DIR{
   // ...
   },
   FILE{
   @Override
   boolean create(File file){
   return result = file.createNewFile​();
   }
   };
   abstract boolean create(File file);
  }
  

• In your code replace the boolean parameter with the enum

  File getOrCreateFile(String fileName, FileOrDir fileOrDir)
  

• Excange the if with a call to the enums method:

   boolean result = fileOrDir.create(file);
   if (!result && !file.exists()) {
   throw new ServiceException(ErrorMessages.FS_MK_ERROR, null, log, file.getAbsolutePath());
   }
   // also delete the if's closing brace!
  

• Replace the literal boolean in the method calls with the appropriate enumconstant:

  public File getOrCreateFolder(String folderName) {
   return getOrCreateFile(folderName, FileOrDir.DIR);
  }
  public File getOrCreateFile(String fileName) {
   return getOrCreateFile(fileName, FileOrDir.FILE);
  

  }

Keep it simple and stupid (KISS)

You use a database to keep track of the files already created. You may have a reason I don't know but as fahr as I see a HashSet object would do this job as well.

Avoid state duplication

You explicitly track the initialized-state of your service in a boolean variable. but on the other hand you check your (other) members for beeing null. This is a duplicated state.

Problem is that sucht stated tent to get out of sync. This is what hapens in your code too: in your shutdown method yos set the menbers to null but you don't update isInitialized.