Today we will discuss a conflict between the design values of keeping things simple, stupid (KISS) and robustness, between underdesign and overdesign.
We were writing a batch Java application and needed to ensure that at maximum one instance is running at a time on the server. A team member had the good idea of using lock files, which indeed worked and helped us a lot. However the original implementation wasn’t very robust, which has cost us valuable people time and expensive context switches due to troubleshooting the damn application rejecting to run and locating the lock file.
As Øyvind Bakksjø of Comoyo has recently explained, a software engineer is distinguished from a mere coder by thinking and caring not only the happy path through the code but also about the unhappy cases. Good engineers think about possible problems and try to handle them gracefuly so that code that depends on them and their users have easier time dealing with problematic situation. Robustness includes catching errors early, handling them in a good way, and providing useful and helpful error messages. On the other hand, simplicity [TBD: Hickey] is a crucial characteristic of systems. It is always too easy to spend too much time on making code bullet-proof instead of focusing the effort somewhere where it would be more valuable to the business.
The overly simple implementation
The original implementation was rather simple:
public class SimpleSingletonBatchJob {
private static boolean getLock() {
File file = new File(LOCK_DIRECTORY+File.separatorChar+Configuration.getGroupPrefix());
try {
return file.createNewFile();
} catch (IOException e) {
return false;
}
}
private static void releaseLock() {
File file = new File(LOCK_DIRECTORY+File.separatorChar+Configuration.getGroupPrefix());
file.delete();
}
public static void exit(int nr) {
releaseLock();
System.exit(nr);
}
public static void main(String[] args) throws IOException {
...
if (! getLock()) { // #1 try to create lock
System.out.println("Already running");
return;
}
... // do the job (may throw exceptions)
releaseLock(); // #2 release lock when done
}
}The main problem is that if the app fails or is killed, it will leave the lock file behind and the next time it will reject to start with an unhelpful error message. You will need to know/read the code to understand how to fix the problem.
It has been argued that such failures and intentional kills would only happen so rarely that the effort to make the code more robust wasn’t justified. However we need to invest very little effort to make the code more friendly and robust, f.ex. by including the lock file path in the error message and explaining why it might be there and how to fix it (such as “if the app isn’t running, the lock is a leftover from a failed run and may be deleted”). Making sure that the file is removed upon failure is a few trivial lines of code that can save some confusion and time. Also, it is worthwile to make it little more robust so that it doesn’t need that many manual interventions – be nice to your ops people. (Which, I hope, is you.)
The more robust implementation
This is the improved version, with a helpful error message and lock removal upon failure:
public class RobustSingletonBatchJob {
// Note: We could use File.deleteOnExit() but the docs says it is not 100% reliable and recommends to
// use java.nio.channels.FileLock; however this code works well enough for us
static synchronized boolean getLock() {
File file = new File(LOCK_DIRECTORY, StaticConfiguration.getGroupPrefix());
try {
// Will try to create path to lockfile if it does not exist.
file.getParentFile().mkdirs(); // #1 Create the lock dir if it doesn't exist
if (file.createNewFile()) {
return true;
} else {
log.info("Lock file " + file.getAbsolutePath() + " already exists."); // #2 Helpful error msg w/ path
return false;
}
} catch (IOException e) {
throw new RuntimeException("Failed to create lock file " + file.getAbsolutePath()
+ " due to " + e + ". Fix the problem and retry."
, e); // #3 Helpful error message with context (file path)
}
}
private synchronized static void releaseLock() {
File file = new File(LOCK_DIRECTORY, StaticConfiguration.getGroupPrefix());
file.delete();
}
public static void main(String[] args) throws Exception {
boolean releaseLockUponCompletion = true;
try {
...
if (! getLock() {
releaseLockUponCompletion = false;
log.error("Lock file is present, exiting."); // Lock path already logged
throw new RuntimeException("Lock file is present"); // throwing is nicer than System.exit/return
}
... // do the job (may throw exceptions)
} finally {
if (releaseLockUponCompletion) {
releaseLock(); // #4 Always release the lock, even upon exceptions
}
}
}Improvements:
- Create the directory storing the locks if it doesn’t exist (its non-existence and resulting confusing error message of “Already running”) has bitten us
- Helpful error message “Lock file <absolute path of the file> already exists.” => easy to copy and paste int rm.
- Helpful error message with the file path and error when we fail to create the lock (lack of space, insufficient directory permissions, …)
- Wrapping the whole main into try – finally and making sure to always delete the lock file
The code still isn’t perfect – if you kill the app, the lock file will be still left behind. There are ways to handle that (f.ex. include the app’s pid in the file, upon start check not only its existence but also that the pid actually exists / is the app) but the cost of implementing them in terms of time and increased complexity is indeed higher than the benefits.
Conclusion
Both KISS and robustness are important objectives and will often be in conflict. Making your code more robust than necessary makes it too complex and costs time and has an (missed) opportunity cost. Making the code too simple costs you or its users time due to troubleshooting. Achieving the right balance requires experience and iterating towards it. If your team can’t find a consensus, it is perhaps better to start with a simpler code and collect hard data on its actual robustness needs rather then overdesigning it upfront. Don’t be perfectionist like me – but also be good to your users and fellow developers. If you can make your app more robust with little effort, do it. If it requires more work, go and collect data to justify (or not) the work.
