Fixing deadlocks in messy code

Question

I'm modifying a bit of seriously complex code and I need to add my own synchronization on top of this.

However, the existing code has about a dozen, if not more, different locks, and my code needs to call some of its methods. I don't really know the order in which the locks are being obtained, nor can I really control it.

So, my question is, what would happen if I replaced all the different locks by a single lock?.Apart from sacrificing granularity, is there any other issue I should be aware of?

Thanks!

Answer 1

If you change all of the synchronized blocks (and methods), and all the other blocking structures, I think you should be fine -- worst case, your app degenerates to having serial execution. But if you only change some of them, you could get deadlock. Consider a scenario where two threads are each acquiring multiple locks:

Thread 1:
    synchronized A
        synchronized B

Thread 2:
    synchronized B
        synchronized C

There's no risk of deadlock here, but if you replace A and C (but not B) with the new, common lock, then you'll have:

Thread 1:
    synchronized L
        synchronized B

Thread 2:
    synchronized B
        synchronized L

... which is the classic deadlock case.

Consider another scenario, where the locks don't provide deadlock themselves, but instead deadlock a blocking class like a CountDownLatch:

Thread 1:
    synchronized A
        latch L.countDown()

Thread 2:
    synchronized B
        latch L.await()

In this case, changing both synchronized blocks to lock on a common lock won't cause deadlock between them, but will cause deadlock if thread 2 gets the lock first: it'll await the latch's countDown, which will never come because thread 1 is blocked at its synchronized entry point. This example applies to other blocking structures, too: semaphores, blocking queues, etc.

Answer 2

I don't think there is any substitute for properly analysing the code. The reason it is horrible is probably because everyone else who has had to modify it has done exactly the same as you and baulked at a proper analysis.

It should be fairly simple to write some logging code that should clarify the locking. Once you can unpick the layers and have a clear picture it should be comparatively simple to replace the whole lot with one modern lock such as a ReadWriteLock or similar.

You may find it useful to take this opportunity to add some test code to exercise your logging in a controlled fashion. A very useful addition to any piece of complex code.