Ales Novak's old blog

Sometimes you just have to sit back and think what the programmer was thinking? I am not sure how the following thing happened, I suspect that somebody did not want to brought in API changes. Anyway, to give you some background - the customer application worked at a "developer workstation" or under light load, thus passing tests in staging environment, but was failing silently, in production environment. The net result was corrupted data. Until somebody inspected data, realizing they were not consistent, there was not any sign that anything went wrong. Here we go.

We start with two methods, writeToDataStore and writeToFileBasedStore:

        public void writeToDataStore(String data, String msg) {
                ...
                writeToFileBasedStore(data, msg);
                ...
        }

        void writeToFileBasedStore(String data, String msg) {
                ...
        }

What happened probably was that somebody has to add third parameter to writeToDataStore, since this was an API method it cannot be just replaced. I can only speculate why they chose not to overload the method, i.e. add writeToDataStore(String data, String msg, String moreData), they were probably afraid of bloated APIs so they came up with the solution, which is here:

        public static Hashtable table = new Hashtable();

        ...
        table.put("datakey", moreData); //*
        writeToDataStore(data, msg);
        ...

        void writeToFileBasedStore(String data, String msg) {
                String moreData = (String) table.get("datakey");
                ...
        }

You should see intent of the developer. However, what he really wrote was code working only in single threaded environment. When deployed to multithreaded environment, e.g. application server, the whole thing just break. You can start to think of your favourite scenario of a race condition. For start, what happens when multiple threads reach and complete line marked with //* simultaneously? Then they all enter writeToDataStore. The last thread's moreData wins and overwrites everything else.

What is the solution for the problem? Well, you can add overloaded, three parameter long version of the method, then four, five, ... long versions as new requirements arise. You can put the critical section in synchronized block, which you should definitely avoid. You can use thread local Hashtable, which is not nice either. I personally would add something like writeToDataStore(Record r) to API - remember OOP basics? Particularly encapsulation?