Updating resources while avoiding race conditions

Question

I am writing a utility class for a game client. This class has two main functions: connect to a game server and retrieve the current game revision, and retrieve a "recipe", which is basically serialized data containing information unique to a revision number. The specifics of the "recipe" are irrelevant for the purposes of this class.

Because one instance of my application can support multiple game sessions, each on a different thread, I need the public methods in this class to be thread-safe. In particular, I am concerned about race conditions in the forceUpdateRevision, getRevision, and getRevision methods. I made the internal update methods synchronize on a private object to ensure that only a single thread can update at the same time, but I am not sure if that is enough to prevent race conditions in the aforementioned methods. I am also not sure if the volatile modifiers are on the cachedRevision and cachedRecipe fields are needed.

By race condition, I am, for example, referring to two threads invoking the forceUpdateRevision method at slightly different times, and the result of updateRevision is different for both threads. In such a case, I fear undefined behavior. I suppose that one solution could be to synchronize the forceUpdateRevision method entirely, rather than the underlying update methods, but I want to minimize how much I synchronize to avoid locking down the entire class by one method call, since updateRevision and updateRecipe, in principle, are completely independent of each other.

Can someone more experienced with thread-safety and race conditions look at my code and see if it can suffer from possible race condition issues?

I have included the entire source code for the class for completeness, though I reckon analyzing the specifics of the internal update methods are not that useful for my question.

public final class Revision {

    public static final SocketAddress ADDRESS = new InetSocketAddress("game.server.address", 11111);
    public static final byte GAME_UPDATE = 1;
    public static final byte GOOD_RESPONSE = 0;
    private static final int REVISION_THRESHOLD = 150;
    private static final Object revisionLock = new Object();
    private static final Object recipeLock = new Object();
    private static volatile int cachedRevision = -1;
    private static volatile Recipe cachedRecipe;

    private Revision() {

    }

    public static void forceUpdateRevision() {
        int revision = updateRevision();
        if (revision != cachedRevision) {
            cachedRevision = revision;
            forceUpdateRecipe();
        }
    }

    public static int getRevision() {
        if (cachedRevision < 1) {
            forceUpdateRevision();
        }
        return cachedRevision;
    }

    public static void forceUpdateRecipe() {
        cachedRecipe = updateRecipe();
    }

    public static Recipe getRecipe() {
        if (cachedRecipe == null) {
            forceUpdateRecipe();
        }
        return cachedRecipe;
    }

    private static int updateRevision() {
        synchronized(revisionLock) {
            File revisionFile = new File("./data/revision.dat");
            int revision = 0;
            if (revisionFile.exists()) {
                DataInputStream in = null;
                try {
                    in = new DataInputStream(new FileInputStream(revisionFile));
                    revision = in.readShort();
                } catch (Exception ignored) {} 
                finally {
                    if (in != null) {
                        try {
                            in.close();
                        } catch (IOException ignored) {}
                    }
                }
            }
            int response = -1;
            while (revision <= REVISION_THRESHOLD) {
                Socket socket = new Socket();
                try {
                    socket.connect(ADDRESS);
                    socket.getOutputStream().write(new byte[] { 
                        GAME_UPDATE, 
                        (byte) ((revision >> 24) & 0xff),
                        (byte) ((revision >> 16) & 0xff), 
                        (byte) ((revision >> 8) & 0xff), 
                        (byte) revision });
                    response = socket.getInputStream().read();
                    if (response == GOOD_RESPONSE) {
                        break;
                    } else {
                        revision++;
                    }
                } catch (Exception ignored) {
                    break;
                } finally {
                    try {
                        socket.close();
                    } catch (IOException ignored) {}
                }
            }
            if (response != GOOD_RESPONSE) {
                return -1;
            }
            if (!createDataDirectory()) {
                return revision;
            }
            DataOutputStream out = null;
            try {
                out = new DataOutputStream(new FileOutputStream(revisionFile));
                out.writeShort((short) revision);
            } catch (Exception ignored) {} 
            finally {
                if (out != null) {
                    try {
                        out.close();
                    } catch (IOException ignored) {}
                }
            }
            return revision;
        }
    }

    private static Recipe updateRecipe() {
        synchronized(recipeLock) {
            File recipeFile = new File("./data/recipe.dat");
            if (!recipeFile.exists()) {
                return null;
            }
            DataInputStream in = null;
            try {
                in = new DataInputStream(new FileInputStream(recipeFile));
                byte[] data = new byte[(int) recipeFile.length()];
                in.readFully(data);
                return new Recipe(data);
            } catch (Exception ignored) {}
            finally {
                if (in != null) {
                    try {
                        in.close();
                    } catch (IOException ignored) {}
                }
            }
            return null;
        }
    }

    private static boolean createDataDirectory() {
        File data = new File("./data/");
        if (!data.exists()) {
            return data.mkdir();
        }
        return true;
    }
}

Answer 1

Your two variables cachedRevision and cachedRecipe must only be accessed inside synchronized blocks - anything else is definitely code smell. Careful analysis might prove you right, but take special care.

Marking static shared variables as volatile is good practice. will ensure that the optimiser does not make assumptions that fail when multi-threaded.

I would repeat the tests currently outside the synchronized blocks again inside the synchronized blocks. This, along with the volatile declarations, avoids a possible race condition when two threads arrive at the first test at near enough to the same time. This might be harmless, but then again it might not be...

Answer 2

With a brief glance there are race conditions in this code. You could definitely call your updateRevision and updateRecipe methods more than necessary because of incorrect locking (possibly worse). If you have to call these methods more than necessary they will be much much slower than adding appropriate locks.

I'm not exactly sure how your files are used/updated it might be a good idea to pull the monitoring of these files out into a separate service. Whenever one of these files changes it will automatically update and clients will always get the most up-to-date data. You could use the WatchService to monitor these files for changes: http://docs.oracle.com/javase/tutorial/essential/io/notification.html

Whenever a Client needs this data they will call a getter which could block if the file just changes and your service is in the middle of updating but most likely it would return the latest data almost instantaneously. It would all depend on how these files are used. This might just be a terrible idea haha...

The goal should be to minimize your HDD and Network access. These resources are expensive to get data from.

On that note:

while (revision <= REVISION_THRESHOLD) {
  Socket socket = new Socket();
  try {
    socket.connect(ADDRESS);
    ...

If you have to open/close a new socket multiple times you could be sucking up some valuable resources. This may be an artifact of how the server works but if possible I would try to connect once and reuse the already open connection to avoid hammering the server. Just a thought.