3
\$\begingroup\$

I wrote some code for my question about deleting temp files that weren't deleted on exit before and would like to get some feedback if I forgot some details or got something wrong.

private static final String PROJECTNAME = "projectname";
private static final String LOCKNAME = "lock_ThisIsAVeryLongFilenameNoOtherApplicationWillUse.lock";

private static void initTempDir(Path tempPath) throws IOException {
    final Path tempFolder = Files.createTempDirectory(tempPath, PROJECTNAME);
    Runtime.getRuntime().addShutdownHook(new Thread(() -> {
        try {
            DeepDelete.deepDelete(tempFolder);
        } catch (IOException e) {
            e.printStackTrace();
        }
    }));

    Timer timer = new Timer(true);
    timer.scheduleAtFixedRate(new TimerTask() {

        @Override
        public void run() {
            Path lockFile = tempFolder.resolve(LOCKNAME);
            try {
                Files.write(lockFile, ByteBuffer.allocate(Long.BYTES).putLong(System.currentTimeMillis()).array());
            } catch (IOException e) {
                System.err.println("Failed to secure avaliability of temp folder, exiting!");
                e.printStackTrace();
                System.exit(-1);
            }
        }

    }, 60_000, 60_000);
    // Make first write manually to be sure the file has been written when
    // old temp files get deleted. Otherwise, the current temp file not
    // having a time stamp (yet) would be classified as old and removed.
    Files.write(tempFolder.resolve(LOCKNAME),
            ByteBuffer.allocate(Long.BYTES).putLong(System.currentTimeMillis()).array());

    long currentTime = System.currentTimeMillis();
    Files.list(tempFolder.getParent())//
            .filter(Files::isDirectory)//
            .filter(p -> p.getFileName().toString().startsWith(PROJECTNAME))//
            .filter(p -> {
                Path lock = p.resolve(LOCKNAME);
                if (!Files.isRegularFile(lock))
                    return true;
                try {
                    return ByteBuffer.wrap(Files.readAllBytes(lock)).getLong() < currentTime - 60_000 * 5;
                } catch (IOException e) {
                    e.printStackTrace();
                    return false;
                }
            })//
            .forEach(p -> {
                try {
                    DeepDelete.deepDelete(p);
                } catch (IOException e) {
                    e.printStackTrace();
                }
            });
}

The method takes in the path to the default temp directory and first initializes the own temp folder with the hardcoded prefix PROJECTNAME. The folder has a file whose name is in the constant LOCKNAME in it. Each minute, the current time is written in this file. In the end of initialization, each folder in the temp directory with the same prefix and with a lock file older than five minutes is considered a leftover from a crash and gets deleted.

\$\endgroup\$
2
\$\begingroup\$

The code looks good and should work properly. Here is some feedback nevertheless. I don't really understand why the file is named as a "lock". A lock file is typically used to prevent multiple instances of a program to run, or synchronise acess to a resource between different processes.

Usage of Timer

The Timer was effectivelly replaces by Executors, more specifically ScheduledThreadPoolExecutor as mentioned in the javadoc of Timer. Oneimmediate advantage is that you can submit tasks as Runnable with a lambda expression.

Deletion of tempFolder

Make sure that the DeepDelete.deepDelete(tempFolder); does not fail if tempFolder doesn't exist. If the application terminates due to an error (such as the failure to write to tempFolder, you would have a second exception.

Duplicated code

Extract the following to a method, which appears twice. It will also do always the error handling:

   private static void writeFile(Path tempFolder) {
    try {
        Path lockFile = tempFolder.resolve(LOCKNAME);
        Files.write(lockFile, ByteBuffer.allocate(Long.BYTES).putLong(System.currentTimeMillis()).array());
    } catch (IOException e) {
        System.err.println("Failed to secure avaliability of temp folder, exiting!");
        e.printStackTrace();
        System.exit(-1);
    }
}

Hardcoded values

Define hardcoded values in constants. It will be easier to maintain.

private static final Integer DELAY_MS = 60_000;
private static final Integer DELETE_THRESHOLD_MS = 5 * 60_000;

Read safely

ByteBuffer.wrap(Files.readAllBytes(lock)).getLong() < currentTime - 60_000 * 5;

The entire file will be read to memory and only then the first Long.BYTES bytes are read as a long. This could be a problem if the file happens to be very large, even though it's not expected. It would be better to only read the first Long.BYTES of the file.

Inconsistency?

Looking at

if (!Files.isRegularFile(lock))
  return true;
try {
  return ByteBuffer.wrap(Files.readAllBytes(lock)).getLong() < currentTime - 60_000 * 5;
} catch (IOException e) {
  e.printStackTrace();
  return false;
}

There seems to be an inconsistency. If the file doesn't exist, the directory is kept. If the file exists but its content is unexpected, we delete the directory. Both situations represent a corrupted temporary folder of the program and it seems to me that both situations should have the same handling.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.