Thoughts on code style

I recently had a nice discussion with a fellow programmer about code style which triggered me to think on this topic and that in turn triggered this post where I hope to outline my thoughts on code style.

Code style is a set of rules used when writing source code. These rules do not usually make any difference to how the code runs but they make a huge difference in how the code looks like. In virtually all software teams I was part of, code style caused some arguments among programmers at one point or another. It is a controversial topic. I think there are two reasons for this.

First, programmers are very particular about how they do things, especially in writing programs. If you dare to ask a programmer to write his code in a certain style, you better have a really good reason for it.

Second, programmers tend to have strong opinions on topics that they understand inside out. If a topic is hard to understand, there has to be an initial upfront investment to understand the topic in order to have an opinion about it. The investment has to be even higher if you want to have a strong opinion because you’ll need to defend your strong opinion against fellow programmers and that requires that you really know what you’re talking about. Code style is both trivial to understand and requires minimal investment in understanding the code style rules. Therefore, programmers tend to have strong opinions about it.

The first camp in code style thinks that code style is extremely important in programming. Code style in programming is like punctuation in writing. No serious writer even considers writing without punctuation and as such, no serious programmer should consider writing code without a common style. According to this camp, the team has to agree on every little detail in code style and the whole team has to adhere to those rules. It’s a sin to not adhere to code style. You write code not for yourself but for other programmers to read and understand and you have a responsibility to make sure your code is understandable and adheres to some common style.

The second camp in code style thinks that code style is utterly useless and sometimes even harmful in programming. They think that it’s hard to find a common code style that all programmers agree and it’s even harder to make programmers adhere to it. At the end of the day, code style has no effect on how a program runs, so spending so much time and effort on something that does not make a difference in reality is useless. Instead, time could be better spent with test coverage or design improvements which do have significant impact on how a program runs. Code style not only causes friction in a team, it also takes valuable time away from useful activities like extending test coverage.

I think both camps have valid points and I can relate to both. However, I don’t think that either camp is totally right. The right attitude about code style lies somewhere in the middle. The first camp is right that we write code for others to read and I agree that we need to pay attention to how we write code. It’s just selfish to have a “my-way or highway” type attitude with code. There’s something more important about code style though. If a team agrees on a common set of rules regarding code style and actually sticks to those rules, it shows that the team is indeed a team that can act like a unit. It shows that people have no egos and the code generated is not considered as yours or mine but rather considered as team’s code both for the team members and to the outside world. This is an important goal to strive for a team and since code style helps in achieving that unity, it should not be dismissed so easily.

On the other hand, the second camp is right that code style should not come at the expense of test coverage or other activities involved in software development. I think this could happen if the team suddenly decides to implement code styles on existing/legacy code. Instead of fixing the code and making it better for existing customers, the team spends the time to make the code look pretty. That’s silly. Similarly, in dysfunctional teams, agreeing on a common code style is a daunting task and it could suck a lot of time and energy out of the team. All of this can be avoided by having a strong figure in the team leading the team to agree on a common code style upfront, however little the commonality might be, and then making sure everyone sticks to these rules throughout the project. Sure that’s some initial investment but once the initial agreement is done and habits start setting in, programmers don’t really have to think about code style, it just happens.

To sum up, I believe that code style is important and all its pros and cons should be considered before adapting or completely abandoning it.

 

When a daemon thread is not so daemon

As you know, threads in Java can be marked as daemon via Thread#setDaemon. The main difference between a normal vs. daemon thread is that the JVM exits when the only threads running are all daemon threads. Daemon threads are usually used as service providers for normal threads running in your application as they don’t affect the shutdown of your application like a normal thread might do.

So, you might be tempted to mark a thread as daemon and blindly assume that it will never block the shutdown of your application. While this is true in most cases, as always, there are exceptions. Consider the following piece of code:

    public static void main(String[] args) {
        Thread t = new Thread(new Runnable() {
            @Override
            public void run() {
                int i = 0;
                while (true) {
                    System.out.println(i++);
                    try {
                        TimeUnit.SECONDS.sleep(1);
                    } catch (InterruptedException e) {
                        e.printStackTrace();
                    }
                }
            }
        });
        t.setDaemon(true);
        t.start();

        try {
            t.join();
        } catch (InterruptedException e) {
            e.printStackTrace();
        }
        System.out.println("end");
    }

A daemon thread is created which simply prints a counter every second. Then, there’s a call to Thread#join to wait for the daemon thread to finish and then the main thread prints “end”. Well, “end” will never be printed and the application will not exit because the daemon thread will continue printing counter and Thread#join will block forever. This might be surprising at first glance. Isn’t this a daemon thread? Isn’t it supposed to not block JVM? Yes, but calling Thread#join on a daemon thread effectively turns the daemon thread into a normal thread.

The lesson here is that marking a thread as daemon is only part of the story. How a daemon thread is used throughout your application is also as important and you need to keep that in mind. Another lesson is that don’t use things that can block forever such as Thread#join. I know you designed your application so perfect that you think it will never block but if something can block in theory, it will block in production sooner or later. That can be quite annoying to customers and support people, so those people before blindly implementing blocking code paths. Instead use non-blocking alternatives like Thread#join(long) and provide a timeout.

 

Origin null is not allowed by Access-Control-Allow-Origin

I got this error in Chrome while testing out a JQuery Mobile application. The problem was that JQuery Mobile app’s index.html was trying to open another local HTML file using “<a href="foo.html …". Opening local files is not allowed by default in Chrome (but works in Firefox and Safari). This is the kind of error you'd get in Chrome:

XMLHttpRequest cannot load file://.../foo.html. 
Origin null is not allowed by Access-Control-Allow-Origin.

To get rid of the error, you can enable local files in Chrome by adding "–allow-file-access-from-files" option (as explained here).

On Mac, I do this but running Chrome using the following command from terminal:

open /Applications/Google\ Chrome.app --args --allow-file-access-from-files

 

Maintaining Legacy Systems

There are a lot of books, blog posts, articles on programming languages, frameworks, design patterns for writing brand new software but not so much on debugging and maintaining legacy systems. This is surprising because a big part of software development is about maintaining legacy systems.

It’s understandable though. Legacy is not exciting but new is, so everybody wants to talk about new things. That’s great but there are a lot of developers in trenches like myself who spend a lot of time working on non-exciting yet essential work like maintaining legacy systems and I feel like we could use some knowledge sharing.

What’s a legacy system?
By legacy systems, I don’t mean some ancient code written in 1980s that you have to maintain (hopefully you don’t!) but rather I mean code that was designed and implemented before your time in your current role. I call these legacy systems from your perspective because you had no input into their design and implementation. You were basically late to the party where all cool design and implementation choices were made and now you are stuck with understanding and maintaining someone else’s thought and implementation flaws. If that person is still around, you’re somewhat lucky. At least, there’s someone to ask questions but most of the time, you’re left on your own to understand what’s going on.

Debugging
Debugging is a prerequisite for understanding and maintaining a legacy system. No matter how simple or complicated a system might be, you will need to step through code to get a general feel for the system. My ex-colleague Jeff Vroom recently had a great post on debugging that covers everything I’d say on that topic.

Maintenance Guidelines
Once debugging is done and you have a good understanding of the system, you need to fix/maintain the system. These are some guidelines I try to remind myself as I’m working with legacy systems.

Have the right mindset
As programmers, we are very particular about how we do things. So quite often, we find someone else’s code ugly, stupid, convoluted, hard to maintain, etc. See code as code only and focus on the problem you’re trying to solve. Nobody cares about how beautiful or ugly the code is according to you but a lot of customers would benefit from the bug fix or feature you’re working on, so make sure you remember that.

Keep bug fixes simple
When fixing a bug in a legacy system, try to keep it as simple as possible. The smaller the fix, the easier it’ll be for others to understand it and the easier it’ll be to merge to other branches. Even though the fix might not be theoretically perfect or up to your standards in terms of style, that’s fine as long as it covers the use case you’re trying to fix. In my experience, the benefits of easily understandable and contained bug fixes outweigh any style or theoretical considerations most of the time.

Add new functionality as a cohesive unit
When you’re working on a feature in a legacy system, try to add the new functionality as a cohesive unit. Don’t scatter your new feature throughout the system. If possible, create new methods within the class you’re editing, or new classes within the system. The more you isolate what you add, the easier time you and others in your team will have later. You’ll be able to test what you added specifically, you’ll be able to take out your code and see if a particular bug still happens and so on.

Resist the urge to refactor
I know if you were there for the initial design and implementation, everything would be better and I know that the code you’re looking at now is not what you want to look at. But despite all that, resist the urge to refactor unless it’s really needed. I’m not saying you shouldn’t change code at all. You should always follow “Leave the code better than you found it” principle but do it incrementally, so people can track what you’re doing. If you refactor bunch of classes in a major way all at the same time, nobody will be able to grasp what you just did. If you’re lucky, there are good tests to catch any errors you introduced but most of the time, that’s not the case, so it’s a big risk to refactor a legacy system in a major way at the same time because you will most certainly introduce errors. Instead accept the system for what it is and do incremental changes. If done consistently, small incremental changes over a period of time do wonders to your legacy code base.

Ask for help but strive for independence
Don’t spend hours and hours trying to understand the logic behind some piece of code, when the original programmer could explain everything in 10 minutes. I know it could be time-consuming and somewhat annoying for the original programmer to spend time explaining his code, but hey, you’re doing them a favor by maintaining their code, so the least they can do is to spend some time in explaining their code. Having said that, your goal is to gradually reduce your dependency on the original programmer. If you sincerely put in the effort to understand the system, it’s just a matter of time.

 

Thread.sleep vs. Object.wait

In my last 2 posts (here and here), I talked about some simple but overlooked topics in Java threading. I want to wrap up this series by pointing out yet another easily overlooked topic related to threading: the subtle difference between Thread.sleep and Object.wait.

In Java, both Thread.sleep and Object.wait make the current thread wait for a specified amount of time. This is useful when the current thread needs to wait for some other thread before it can proceed. I sometimes see developers use these interchangeably in close proximity in code but this can be problematic as the two methods are quite different.

Consider the example from my previous post. In that example, thread1 acquired a lock, slept for a minute while thread2 was blocked waiting for the same lock. It used TimeUnit.MINUTES.sleep(1) which uses Thread.sleep to make thread1 wait. Let’s change it to use Object.wait and see what happens:

private static void test() {
    final Object lock = new Object();

    Thread thread1 = new Thread(new Runnable() {
        @Override public void run() {
            synchronized (lock) {
                System.out.println("Thread1 acquired lock");
                try {
                    lock.wait(1 * 60 * 1000);
                    //TimeUnit.MINUTES.sleep(1);
                } catch (InterruptedException ignore) {}
            }
        }

    });
    thread1.start();

    Thread thread2 = new Thread(new Runnable() {
        @Override public void run() {
            synchronized (lock) {
                System.out.println("Thread2 acquired lock");
            }
        }
    });
    thread2.start();
}

Once you run this sample, you’ll see the following right away:

Thread1 acquired lock
Thread2 acquired lock

So what happened? When thread1 went to sleep, it released the lock and thread2 acquired it right away. Unlike Thread.sleep (where locks are not released) in Object.wait, locks are released as the thread goes to sleep. In some situations, this might be appropriate, in others, maybe not, so this is an important thing to keep in mind when deciding which sleep method to use.

 

Block != Deadlock

I sometimes hear programmers use the terms “threads are blocked” and “threads are deadlocked” interchangeably even though block and deadlock are two very different notions. So, in this post, I want to briefly highlight the difference between blocks and deadlocks to set the record straight.

In my previous post, I talked about thread deadlocks. Thread blocks, on the other hand, happen when one thread acquires a lock and holds onto it while the second thread waits for the same lock. In this case, the second thread is blocked until the first thread releases the lock. Here’s a quick example:

    private static void test() {
        final Object lock = new Object();

        Thread thread1 = new Thread(new Runnable() {
            @Override public void run() {
                synchronized (lock) {
                    System.out.println("Thread1 acquired lock");
                    try {
                        TimeUnit.MINUTES.sleep(1);
                    } catch (InterruptedException ignore) {}
                }
            }

        });
        thread1.start();

        Thread thread2 = new Thread(new Runnable() {
            @Override public void run() {
                synchronized (lock) {
                    System.out.println("Thread2 acquired lock");
                }
            }
        });
        thread2.start();
    }

Notice how Thread1 acquires the lock and does not release it for a minute. Once you run this sample, you’ll see “Thread1 acquired lock” but Thread2 does not acquire the lock for a whole minute. In that 1 minute, you can see that Thread2 is blocked in the JVM thread dump:

"Thread-2" prio=5 tid=101978000 nid=0x10a704000 waiting for monitor entry [10a703000]
   java.lang.Thread.State: BLOCKED (on object monitor)
	at BlockTest$2.run(BlockTest.java:32)
	- waiting to lock <7f3112a18> (a java.lang.Object)
	at java.lang.Thread.run(Thread.java:680)

Thread2 will eventually be unblocked, in this case, after 1 minute. So, unlike deadlocks, there’s some hope in blocks but to the end user deadlocks and blocks basically manifest themselves as unresponsive application, especially if the blocks are too frequent and long.

However, there is no excuse for programmers to confuse blocks and deadlocks as they are 2 completely different notions

 

Deadlock Detection in Java

In this post, I want to talk about deadlock detection in Java and some pitfalls you might run into.

What is a deadlock?

Every programmer knows what a deadlock is but for completeness sake, I’ll give a brief description. Imagine you have two threads, thread1 and thread2. Thread1 acquires lock1 and is about to acquire lock2 while thread2 acquires lock2 and is about to acquire lock1. In this case, threads are deadlocked because they are each waiting for the other thread to release the other lock and your application just hangs. Deadlocks usually occur when the order of locks is not consistent throughout the application.

Deadlock sample with regular sync blocks

Here is a a quick sample that shows the deadlock situation explained above with good old sync blocks.

    private static void test1() {
        final Object lock1 = new Object();
        final Object lock2 = new Object();

        Thread thread1 = new Thread(new Runnable() {
            @Override public void run() {
                synchronized (lock1) {
                    System.out.println("Thread1 acquired lock1");
                    try {
                        TimeUnit.MILLISECONDS.sleep(50);
                    } catch (InterruptedException ignore) {}
                    synchronized (lock2) {
                        System.out.println("Thread1 acquired lock2");
                    }
                }
            }

        });
        thread1.start();

        Thread thread2 = new Thread(new Runnable() {
            @Override public void run() {
                synchronized (lock2) {
                    System.out.println("Thread2 acquired lock2");
                    try {
                        TimeUnit.MILLISECONDS.sleep(50);
                    } catch (InterruptedException ignore) {}
                    synchronized (lock1) {
                        System.out.println("Thread2 acquired lock1");
                    }
                }
            }
        });
        thread2.start();

        // Wait a little for threads to deadlock.
        try {
            TimeUnit.MILLISECONDS.sleep(100);
        } catch (InterruptedException ignore) {}
    }

The sample prints the following and then just hangs due to deadlocked threads:

Thread1 acquired lock1
Thread2 acquired lock2

Manual Deadlock Detection

While your app is hanging like in the example above, you can get a thread dump and see the deadlocked threads. For example, on Mac, you can either do Ctrl-\ or simply use jstack and process id to get the thread dump which makes it very obvious where the deadlock is. In this example, the thread dump looks like this:

Found one Java-level deadlock:
=============================
"Thread-2":
  waiting to lock monitor 102054308 (object 7f3113800, a java.lang.Object),
  which is held by "Thread-1"
"Thread-1":
  waiting to lock monitor 1020348b8 (object 7f3113810, a java.lang.Object),
  which is held by "Thread-2"

Java stack information for the threads listed above:
===================================================
"Thread-2":
	at DeadlockTest$2.run(DeadlockTest.java:42)
	- waiting to lock <7f3113800> (a java.lang.Object)
	- locked <7f3113810> (a java.lang.Object)
	at java.lang.Thread.run(Thread.java:680)
"Thread-1":
	at DeadlockTest$1.run(DeadlockTest.java:26)
	- waiting to lock <7f3113810> (a java.lang.Object)
	- locked <7f3113800> (a java.lang.Object)
	at java.lang.Thread.run(Thread.java:680)

Found 1 deadlock.

Programmatic Deadlock Detection

In more recent JDKs, there’s programmatic deadlock detection where you can get more information about the deadlocked threads and even stacktraces leading to deadlock as explained in this stackoverflow.com entry. In our example, let’s just detect the number of blocked threads in our application:

    private static void detectDeadlock() {
        ThreadMXBean threadBean = ManagementFactory.getThreadMXBean();
        long[] threadIds = threadBean.findMonitorDeadlockedThreads();
        int deadlockedThreads = threadIds != null? threadIds.length : 0;
        System.out.println("Number of deadlocked threads: " + deadlockedThreads);
    }

If you call detectDeadlock to the end of the test1 method, you get the following nice output with the number of deadlocked threads:

Thread1 acquired lock1
Thread2 acquired lock2
Number of deadlocked threads: 2

Deadlock sample with other locks

This is all nice but what about advanced locks like ReentrantLock, does programmatic detection work in that case? Let’s use this sample:

    private static void test2() {
        final ReentrantLock lock1 = new ReentrantLock();
        final ReentrantLock lock2 = new ReentrantLock();

        Thread thread1 = new Thread(new Runnable() {
            @Override public void run() {
                try {
                    lock1.lock();
                    System.out.println("Thread1 acquired lock1");
                    try {
                        TimeUnit.MILLISECONDS.sleep(50);
                    } catch (InterruptedException ignore) {}
                    lock2.lock();
                    System.out.println("Thread1 acquired lock2");
                }
                finally {
                    lock2.unlock();
                    lock1.unlock();
                }
            }
        });
        thread1.start();

        Thread thread2 = new Thread(new Runnable() {
            @Override public void run() {
                try {
                    lock2.lock();
                    System.out.println("Thread2 acquired lock2");
                    try {
                        TimeUnit.MILLISECONDS.sleep(50);
                    } catch (InterruptedException ignore) {}
                    lock1.lock();
                    System.out.println("Thread2 acquired lock1");
                }
                finally {
                    lock1.unlock();
                    lock2.unlock();
                }
            }
        });
        thread2.start();

        // Wait a little for threads to deadlock.
        try {
            TimeUnit.MILLISECONDS.sleep(100);
        } catch (InterruptedException ignore) {}

        detectDeadlock();
    }

And the output we get is:

Thread1 acquired lock1
Thread2 acquired lock2
Number of deadlocked threads: 0

Whoa! What happened? Deadlock detection does not work with ReentrantLocks?

findMonitorDeadlockedThreads vs. findDeadlockedThreads

Well, part of the blame is mine. There are two methods in ThreadMXBean to detect deadlocks: findMonitorDeadlockedThreads and findDeadlockedThreads. I carelessly used the former but there’s a subtle difference between the two.

As JavaDocs point out, findMonitorDeadlockThreads “Finds cycles of threads that are in deadlock waiting to acquire object monitors” whereas findDeadlockedThreads “Finds cycles of threads that are in deadlock waiting to acquire object monitors or ownable synchronizers“.

What’s an ownable synchornizer? Again according to JavaDocs: “An ownable synchronizer is a synchronizer that may be exclusively owned by a thread and uses AbstractOwnableSynchronizer (or its subclass) to implement its synchronization property. ReentrantLock and ReentrantReadWriteLock are two examples of ownable synchronizers provided by the platform.”

In plain English, if you want to detect ReentrankLock or ReentrantReadWriteLock, make sure you use findMonitorThreads method. If we change our deadlock method to the following:

    private static void detectDeadlock() {
        ThreadMXBean threadBean = ManagementFactory.getThreadMXBean();
        long[] threadIds = threadBean.findDeadlockedThreads();
        int deadlockedThreads = threadIds != null? threadIds.length : 0;
        System.out.println("Number of deadlocked threads: " + deadlockedThreads);
    }

We get the following expected output with test2:

Thread1 acquired lock1
Thread2 acquired lock2
Number of deadlocked threads: 2

So, you might ask at this point, why even bother with findMonitorDeadlockedThreads? Well, findDeadlockedThreads was added in JDK6. So, if your project is using JDK5 still, you need to use findMonitorDeadlockedThreads and hopefully this post will remind you the caveats of using findMonitorDeadlockedThreads.