Is this a safe way to synchronize a block of code?

Question

In our application, there's a section of code that runs continuously reading and adjusting files. Just to give you a sense of what's going on:

public void run() {
    try {
        while(true) { //Yeah, I know...
            Path currentFileName = getNextFile();
            String string = readFile(currentFileName);
            Files.deleteFile(currentFileName);
            string = string.replaceAll("Hello", "Blarg");
            writeFile(currentFileName);
        }
    } catch (Exception e) {
        System.err.println("It's all ogre now.");
        e.printStackTrace(System.err);
    }
}

Elsewhere in our code is a method that might-but-usually-doesn't run on the same thread as the above code, that we use to exit the application.

private void shutdown() {
    if(fileReader != null)
        fileReader = null;

    System.exit(0); //Don't blame me, I didn't write this code
}

As might be obvious, there's a potential Race Condition in this code, whereby if shutdown() is called between retrieving the file and then writing it back out, it'll potentially cause the file to be completely lost. This is, obviously, undesirable behavior.

There's a thousand issues with this code (outside the scope of just what I've shown here), but the main issue I need to resolve is the bad behavior where processing a file can be cut off halfway through with no recourse. My proposed solution involves simply wrapping the while loop in a synchronized block and putting a block around the System.exit call in shutdown.

So my changed code would look like this:

private Object monitor = new Object();
public void run() {
    try {
        while(true) {
            synchronized(monitor) {
                Path currentFileName = getNextFile();
                String string = readFile(currentFileName);
                Files.deleteFile(currentFileName);
                string = string.replaceAll("Hello", "Blarg");
                writeFile(currentFileName);
            }
        }
    } catch (Exception e) {
        System.err.println("It's all ogre now.");
        e.printStackTrace(System.err);
    }
}

private void shutdown() {
    synchronized(monitor) {
        if(fileReader != null)
            fileReader = null;

         System.exit(0);
    }
}

The main thing I'm worried about is the System.exit(0); call, where I'm not certain about the total behavior behind the scenes of the call. Is there a risk that a side-effect of System.exit will be to release the lock on monitor, and thus risk the contents of the loop in run being executed partially before System.exit has caused the JVM to halt? Or will this code guarantee that the execution will never attempt a shutdown part-way through the handling of an individual file?

Note: before some armchair programmers step in with alternatives, I'd like to point out that what I've put here is a truncated version of about 4000 lines of code all stashed in a single class. Yes, it is awful. Yes, it makes me regret my chosen profession. I am not here looking for alternate solutions to this problem, I am only trying to determine if this specific solution will work, or if there's some critical flaw that would preclude it from ever working as I expect.

Answer 1

Or will this code guarantee that the execution will never attempt a shutdown part-way through the handling of an individual file?

This code guarantees that a shutdown initiated here will not occur part-way through handling of an individual file. Perhaps obviously, somewhere else in your code could invoke System.exit, and you have no protection from that.

You might want to consider preventing System.exit from being invoked, and then cause your code to shut down gracefully (i.e. by normal completion of the main method).

Answer 2

In case you really have multiple threads calling the different methods, using synchronized like this is actually a clever idea, as it takes care of that "multiple threads" thing.

You could consider reducing the scope of the first block:

Path currentFileName = getNextFile();
String string = readFile(currentFileName);
synchronized(monitor) {

reading the file alone shouldn't be a problem. (of course, unless your code here has to guarantee that a Path returned by getNextFile() gets fully processed).

Answer 3

If the code is executing in the synchronized block and the blocks synchronize on the same object, and the methods invoked in the synchronized block in the while loop operate entirely on the same thread as their caller, then there is no risk of the file-related processes being interrupted by that invocation of System.exit.

This said, it does look like a debatable patch only slightly improving debatable code.

There may also be a more concrete risk of starvation, as the while loop as displayed just seems to spam those file operations as fast as possible, hence the attempt to acquire the lock when exiting may not succeed.

A general direction to explore would be to transform the infinite while loop into a ScheduledExecutorService + Runnable, executing every x amount of time with its own monitor to prevent overlapping operations on the same file(s), and gracefully terminating it when the shutdown method is invoked.

Answer 4

You could use a shutdown hook. From the javadocs:

A shutdown hook is simply an initialized but unstarted thread. When the virtual machine begins its shutdown sequence it will start all registered shutdown hooks in some unspecified order and let them run concurrently.

https://docs.oracle.com/javase/7/docs/api/java/lang/Runtime.html#addShutdownHook(java.lang.Thread)

From this, you could provide a shutdown hook from your file class as follows:

public Thread getShutdownHook() {
    return new Thread(() -> {
        synchronized (monitor) {
            // gracefully handle the file object
        }
    });
}

This will be called when Runtime.getRuntime().exit() is called (which is called by System.exit()). Since it is also synchronized on the monitor object, if the file is in use by the other thread, the shutdown hook will block until it is free.

Answer 5

Even if you cope with concurrent System.exit(), other reasons remain which can lead to sudden break of execution, like power outage or hardware error.

So you better do not remove the file before the new version is written. Do something like:

Path currentFileName = getNextFile();
Path tmpFileName = currentFileName+suffix();
String string = readFile(currentFileName);
string = string.replaceAll("Hello", "Blarg");
writeFile(tmpFileName);
Files.move(tmpFileName, currentFileName);