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).