Java function that blocks until a specific file is deleted

Question

Created for a console application that will be run in the background on a linux system, giving me a nice way to gracefully shut it down by simply deleting a file (which can be done via script/etc.)

public class BlockOnRunFile {
    private File watchedFile;
    private Path path;
    private WatchService watcher;

    public void end() {
        watchedFile.delete();
    }

    public BlockOnRunFile(String runFilePath) throws IOException {
        watcher = FileSystems.getDefault().newWatchService();
        path = FileSystems.getDefault().getPath(
                runFilePath.substring(0, runFilePath.lastIndexOf(FileSystems
                        .getDefault().getSeparator())));
        watchedFile = new File(runFilePath);
        watchedFile.createNewFile();
    }

    public void block() {
        try {
            WatchKey key;
            key = path.register(watcher, StandardWatchEventKinds.ENTRY_DELETE);

            // stall until the game is supposed to end
            // reset key to allow new events to be detected
            while (key.reset()) {

                // key = watcher.take();
                try {
                    for (WatchEvent<?> event : key.pollEvents()) {
                        WatchEvent.Kind<?> kind = event.kind();
                        if (kind == StandardWatchEventKinds.OVERFLOW) {
                            Common.log.logMessage("File watcher overflow",
                                    LogLevel.INFO);
                            if (!watchedFile.exists()) {
                                // do nothing
                                ;
                            }
                            break;
                        }
                        if (kind == StandardWatchEventKinds.ENTRY_DELETE) {
                            @SuppressWarnings("unchecked")
                            WatchEvent<Path> ev = (WatchEvent<Path>) event;
                            Path filename = ev.context();
                            if (filename
                                    .toAbsolutePath()
                                    .toString()
                                    .equals(watchedFile.getAbsolutePath()
                                            .toString())) {
                                watcher.close();
                                break;
                            }
                        }
                    }
                    Thread.sleep(1000);   // prevent CPU burn, worst-case: 1 second delay on shutdown
                } catch (Exception e) {
                    watcher.close();
                    Common.log.logMessage(e, LogLevel.INFO);
                    continue;
                }
            }// end while loop
        } catch (IOException e1) {
            Common.log.logMessage(e1, LogLevel.ERROR);
        }
    }
}

It works, and I don't expect there to be any large amount of file operations in the directory where the file exists, but I would still like to know of any possible issues (I intend to automate it via some scripts/crontab and make sure it's robust enough)

Answer 1

This is an interesting concept. I would link it in with, say, a pid file, and have the program terminate itself when the pid file is deleted.... and also delete it's own pid if it terminates early. I think that's why you have your end() method....

General

public class BlockOnRunFile {
    private File watchedFile;
    private Path path;
    private WatchService watcher;

The above variables should all be final too. Also, there's an uncomfortable mix of the "old" File-based system, and the "new" Path based one. I recommend that you use Path, and stick to it.

Simplifications

This code is.... ugly.

path = FileSystems.getDefault().getPath(
        runFilePath.substring(0, runFilePath.lastIndexOf(FileSystems
                .getDefault().getSeparator())));
watchedFile = new File(runFilePath);
watchedFile.createNewFile();

It can be simplified a lot:

Path watchedFile = Paths.get(runFilePath).toAbsolutePath();
Path runDir = watchedFile.getParent();
Files.deleteIfExists(watchedFile);
Files.createFile(watchedFile);

Note, there is no actual File instance in there, all just paths.... Also, there is no Watcher in there. There is no need to create the Watcher in the constructor, it is only used in the block() method, so use it there.

In your block() method, the try/catch is good, but I would add a finally:

} finally {
    Files.deleteIfExists(watchedFile);
}

The Watcher should be added as a resource to the top of that try.... and it auto-closes that way...

Also, about simplification, this code should be 1 line:

WatchKey key;
key = path.register(watcher, StandardWatchEventKinds.ENTRY_DELETE);

like:

WatchKey key = runDir.register(watcher, StandardWatchEventKinds.ENTRY_DELETE);

Then, your code body, the guts, is a little more complicated than necessary, and has a bug....

The first thing is that the inner try/catch is unnecessary, and buggy. You cannot close() the watcher and then try to reset the key, and wait for more. That will just fail. The code will simply terminate the loop, yet you have a continue statement. It's unintuitive. I would simply remove the try/catch entirely.

The second bug is in the "OVERFLOW" code. Your code tests for if the file does not exist, but it should only continue if it does exist.

Finally, why do you have the take() commented out... and use a Thread.sleep()? The take() is a blocking operation, and will 'sleep' until there is an event ready. Use the logic in the take(), and trust it...

Oh, and your handling of InterruptedException is not good.....

Finally, you go to a lot of effort to make sure that every event in the directory is 'handled', but, there is no need to handle the events. All we need to do is to make sure our sentry file is still there. If it is gone, then we die. We don't care about any other files.... this essentially removes the need for the Overflow and other conditions....

Suggestions

With the 'finally' block, the above fixes, and a little bit of "return" instead of "break", your code becomes:

import java.io.IOException;
import java.nio.file.FileSystems;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.nio.file.StandardWatchEventKinds;
import java.nio.file.WatchKey;
import java.nio.file.WatchService;

public class BlockOnRunFile {
    private final Path watchedFile;

    public BlockOnRunFile(String runFilePath) throws IOException {
        watchedFile = Paths.get(runFilePath).toAbsolutePath();
        Files.deleteIfExists(watchedFile);
        // create entire directory tree, if possible, to create our watch file
        // in.
        Files.createDirectories(watchedFile.getParent());
        Files.createFile(watchedFile);

    }

    public void end() throws IOException {
        Files.deleteIfExists(watchedFile);
    }

    public void block() {
        try (WatchService watcher = FileSystems.getDefault().newWatchService()) {
            final WatchKey key = watchedFile.getParent().register(watcher,
                    StandardWatchEventKinds.ENTRY_DELETE);

            // stall until the game is supposed to end
            // reset key to allow new events to be detected
            while (key.reset()) {

                // wait for a file to be deleted (or an overflow....)
                if (key != watcher.take()) {
                    throw new IllegalStateException(
                            "Only our key is registered, only it should be taken");
                }

                // now, we know something has changed in the directory, all we
                // care about though, is if our file exists.
                if (!Files.exists(watchedFile)) {
                    return;
                }

            }
        } catch (IOException e1) {
            Common.log.logMessage(e1, LogLevel.ERROR);
        } catch (InterruptedException e) {
            // propogate an interrupt... we can't handle it here.....
            // just let the file be removed, and we die....
            Thread.currentThread().interrupt();
            Common.log.logMessage(e, LogLevel.WARN);
        } finally {
            try {
                Files.deleteIfExists(watchedFile);
            } catch (IOException e) {
                // unable to delete the sentry file.....
                Common.log.logMessage(e, LogLevel.WARN);
            }
        }
    }
}

Answer 2

I'll address functionality more than style, as I feel @rolfl has addressed style already.

You don't actually block

What you do is poll every 1 second. However the WatchService API is equipped to do actual blocking. I gather you tried doing that, but gave up (?) from the fact that this comment is left

            // key = watcher.take();

That is an actual blocking call. Assuming watchedFile is defined as a Pathrather than as a File the block method should be like this :

public void block() {
    try (WatchService watcher = FileSystems.getDefault().newWatchService()) {
        WatchKey key = path.register(watcher, ENTRY_DELETE, OVERFLOW);
        while (key.reset()) {
            try {
                key = watcher.take(); // this will actually block
                List<WatchEvent<?>> watchEvents = key.pollEvents();
                if (watchEvents.stream().filter(event -> ENTRY_DELETE.equals(event.kind())).anyMatch(event -> watchedFile.getFileName().equals(event.context()))) {
                    key.cancel();
                }
            } catch (InterruptedException e) {
                Thread.currentThread().interrupt();
                key.cancel();
            }
        }
    } catch (IOException e) {
        throw new RuntimeException(e);
    }
}

This leaves out handling OVERFLOW for brevity.

Inject the FileSystem

As it is your code only works on the default file system. That is mostly fine, but refactoring it to have the FileSystem injected is not too hard, and enables testing it with the google's jimfs.