The demo I got most recently pulled into, involving protocols and file formats that I don't know, as well as working over a weekend, is finished. But now there's another, totally different, demo that needs to be set up by the end of the week.

On another front, the caching optimization that I've also been pulled into is being tested. First, people reviewing the code had problems with

    Object lock = new Object();
    synchronized (lock) {
       ...
    }

because, based on that snippet, they said that the synchronized statement does nothing. However, if they had looked inside the synchronized statement, they would have seen the lock object being put inside a static cache, and, if they had looked at the code just outside of the snippet, they would have seen code extracting stuff from the cache. The snippet is inside the if block that occurs when extracting null from the cache. One of the else blocks would have done a synchronized on a lock object extracted from the cache to block on it instead of duplicating the work done by the thread that created the lock object.

In any case, there was a problem with the code. It seems that the code that generated the stuff in the cache also had some side-effects, so that when there was a cache hit, it would fail due to the side-effects not being done. But the caching code was being blamed, because it didn't fail in the "single-threaded case", which apparently didn't test the cache hit case.

There was a real problem with the caching code that was revealed. If the thread generating the cached data threw an exception, it would release the lock, but the lock object would still be in the cache, so other threads would endlessly loop, waiting for the first thread to replace the lock object with the cached data.

This is what the code should be with the proper clean-up:

for (;;) {
    Object entry;
    synchronized (cache) {
        entry = cache.get(key);
    }
    if (entry == null) {
        Object lock = new Object();
        synchronized (lock) {
            try {
                synchronized (cache) {
                    if (cache.get(key) != null) {
                        lock = null;
                        continue;
                    }
                    cache.put(key, lock);
                }
                CacheData data = computeData(key);
                synchronized (cache) {
                    cache.put(key, data);
                }
                lock = null;
                return data;
            } finally {
                if (lock != null) {
                    synchronized (cache) {
                        cache.remove(key);
                    }
                }
            }
        }
    } else if (entry instanceof CacheData) {
        return (CacheData) entry;
    } else {
        synchronized (entry) {}
    }
}

Note that without the finally block, if computeData() were to throw an exception, all other lookups with that key would go into infinite loops.

The point of this code is to call computeData() only once per key, to avoid load spikes when starting up. Another approach, which was also implemented, is to preload the cache. But that code is much more uninteresting.

Unfortunately, the actual code was made more complicated, though the caching logic should be the same.

In this caching code that I worked on due to a performance problem, I got some feedback proposing to take out a bunch of the synchronized blocks because of some ignorant reasoning that the synchronized blocks don't do anything except hurt performance. My response pointed out why each of the synchronized blocks was necessary, and, for one instance, quoting the javadoc for java.util.HashMap that says, "If multiple threads access a hash map concurrently, and at least one of the threads modifies the map structurally, it must be synchronized externally."

For the synchronized HashMap access, I suppose using java.util.concurrent.ConcurrentHashMap would allow the removal of the synchronization. Since ConcurrentHashMap was introduced in Java 1.5, and the code still had to be Java 1.4, I didn't mention it in my response.

I don't know what will come of this. I don't know the person making the proposal, but I suspect that it is someone who is relatively junior. I hope he or she will learn from this.

There was some new code recently added passing around an extra parameter that did nothing more than stick an extra string in the logging. The reasoning is

...it is pretty much impossible to trace the flow of a single request to the server when other requests are coming in. I propose we add a simple prefix to each log [...] that will contain a unique ID per request that can be grepped in the logs...

But it's actually not impossible to follow a single request, since each log entry includes the name of the thread. But if it helps him, it's fine with me as long as I don't have to maintain that code.

It's like that other almost as silly request from someone who has since left to rename certain threads because he wanted them to have certain names in the logs. Personally, I prefer keeping the thread names from the JVM or from the servlet engine, because they will be unique. I had to rework that thread renaming code after adding some other feature caused thread name collisions.

I noticed an exception handler in a listener that has been added to a service. Why is there a System.exit?

        } catch (IOException e) {
            System.err.println("Could not listen on port: " + portNumber + ".");
            System.exit(-1);
        }

Any IOException in the listener will halt the entire system. I don't know if that is intentional. At the moment, this listener is pretty much a prototype. The rest of the system is running in production. I imagine that testers would be annoyed if the System.exit blocks the testing of everything else if there are problems with the new listener.

Actually, the webapp would probably just respawn repeatedly until the problem fixes itself, which would probably be never.

So I got thrown into this other project that I know almost nothing about, doing stuff with protocols that I know nothing about, and working with data formats that I know nothing about. Fortunately, dealing with the stuff I know nothing about either involves making library calls or porting existing code. Not that making library calls is simple matter when lacking the understanding of what the calls are supposed to do, which means the code winds up being blindly based on sample code.

While porting code that does stuff that I don't understand, and I came across this function:

int GetNextStartCode(Byte_t* pData, size_t bufSize, int startIndex) 
{
    uint32_t dataLen = bufSize;
    uint32_t codeMask = 0x0ffffff;
    uint32_t codeWindow = codeMask;//24 bit window. looking for
    uint32_t startCode = 0x000001;
    for (int i=startIndex; i<dataLen; i++) 
    {
        codeWindow = (codeWindow<<8 | pData[i]) & codeMask;
        if (codeWindow == startCode) 
        {
            assert(i>2);
            return i-3;
        }
    }
    return -1;
}

After simple-mindedly rewriting it in Java, I realized what it was actually doing, and figured it would be clearer to write it like this:

    private int getNextStartCode(byte[] data, int start) {
        for (int i = start; i < data.length-2; i++) {
            if (data[i] == 0 && data[i+1] == 0 && data[i+2] == 1)
                return i;
        }
        return -1;
    }

After looking even more at the code, I realized that the GetNextStartCode was really only being used once with startIndex = 0, and that the subsequent calls were ultimately not used, with bufSize being used in the end.