We've all heard of threads. No, not the stuff hanging loosely from your clothes. I mean threads, as in multitasking. Most modern languages have all sorts of nifty facilities that allow you to create, manipulate and destroy them at will and with minimal effort. There are even abstractions that will manage a set of threads for you, so that you can spawn a bunch of tasks, and let them tell you when they're done. You can synchronize them yourself. You can put up cyclic barriers to make them all wait at a specific point in the code. You can make them return a value when they're done. Or you can just spawn them and let them run all by their lonesome. Of course, not everyone trusts the built-in facilities... Now you might expect this sort of thing from Joe Offshore, but not from certain huge, blue companies.

Baron inherited something written by such a huge, blue company. Basically, it monitors a database, does some work for each changed record and then deletes the record from the database. Unfortunately, this little beast suffers from horrific performance problems and requires frequent server reboots to get things running again.

One of the original support folks from Huge Blue asked if increasing the number of threads helped. After a couple of months of trial and error, it was determined that 5 threads was optimal, but it only increased the reboot interval; it did not really fix the problem.

Finally, Baron dug into the code and found a snakepit-o-wtf™ so deep and pervasive, that you should be required to take a sedative before entering the code...

Some of the more egregious offences include:

  • The thread pool manager is not thread safe
  • Exceptions were not caught, but sent to the console instead of the log4j log file
  • The thread pool manager manages creation of threads up to a certain count, but the threads themselves decrement that count upon completion and remove themselves from the array - unsynchronized
  • As threads are removed from the array, the application becomes single threaded over time 

abstract class ThreadPoolManager {
    static String SQL_LOG = "sql.";  /* NOI18N */
    private static final Log logger = LogFactory.getLog(ThreadPoolManager.class);

    protected static int poolSize;
    protected static int largestPoolSize;
    protected static int minPoolSize;

    protected List workers = new ArrayList();

    private long completedTaskCount;

    protected Runnable execute(Runnable r) {
        Runnable next = addIfUnderMaximumPoolSize(r);
        return next;
    }

    private Thread addThread(Runnable r) {
        return new Thread(r);
    }

    private Runnable addIfUnderMaximumPoolSize(Runnable firstTask) {
        Thread t = null;
        Runnable next = null;
        try {
            if (poolSize < largestPoolSize) {
                next = firstTask;
                t = addThread(next);
            }
        } finally {
            // intentionally left blank
        }
        if (t == null) {
            return null;
        }
        t.start();
        return next;
    }

    private void runTask(Runnable task) {
        try {
            boolean ran = false;
            try {
                task.run();
                ran = true;
                ++completedTaskCount;
            } catch (RuntimeException ex) {
                logger.error("Error occured in afterExecute method.", ex);
                throw ex;
            }
        } finally {
            logger.debug("<-- runTask(task)");
        }
    }

    /**
     * Allow the subclass to request that a thread be terminated
     */
    protected void workerDone(Worker w) {
        logger.debug("--> workerDone(Worker w)");
        try {
            workers.remove(w);
            if (--poolSize > 0) {
                w.thread.interrupt();
                return;
            }
        } finally {
            logger.debug("<-- workerDone(Worker w)");
            w.thread.interrupt();
        }
    }

    private class Worker implements Runnable {
        /**
         * Initial task to run before entering run loop
         */
        private Runnable firstTask;
        /**
         * Thread this worker is running in.  Acts as a final field,
         * but cannot be set until thread is created.
         */
        Thread thread;

        Worker(Runnable r) {
            this.firstTask = r;
        }

        /**
         * Main run loop
         */
        public void run() {
            logger.debug("--> run()");
            try {
                Runnable task = firstTask;
                firstTask = null;
                Thread.interrupted();
                while (task != null) {
                    runTask(task);
                    task = null; // unnecessary but can help GC
                }
            } catch (Exception ie) {
                // fall through
            } finally {
                workerDone(this);
                logger.debug("<-- run()");
            }
        }
    } //end of class Worker

}

How many more can you find?

[Advertisement] BuildMaster allows you to create a self-service release management platform that allows different teams to manage their applications. Explore how!