locking anti-pattern

[Adam Heath-2]

public synchronized void fooMethod(Map map) {
    // do something
    boolean result = map.containsKey("foo");
    // do something else
}


public class MyMap extends Map {
    public synchronized Object get(Object key) {
        // do something
        fooMethod(this);
        // do something else
        return keyValue;
    }
}


The above seemingly simple code is dead-lock prone.  There is no way
to know what map.get() is actually doing.  It could post the update to
another thread, waiting for processing to occur(like some kind of
database transaction threadpool worker).

Basically, if you use synchronized, you have to be *very* careful
about the methods you call while you have a lock held.  There is no
guarantee about what locks the called code may be taking.  It might
even work now, but the library could change in the future to do
different locking.

Btw, this is straight out of the Java Concurrency in Practice book.

ps: I was adding my commons-vfs code, and running the tests cases
against them, and got a dead-lock.  So I decided to post this
anti-pattern description here.

pps: If you have no synchronized at all, and use non-blocking
algorithms, then the dead-lock problem goes away.

[Adrian Crum]

Adam Heath wrote:

> public synchronized void fooMethod(Map map) {
>     // do something
>     boolean result = map.containsKey("foo");
>     // do something else
> }
>
>
> public class MyMap extends Map {
>     public synchronized Object get(Object key) {
>         // do something
>         fooMethod(this);
>         // do something else
>         return keyValue;
>     }
> }
>
>
> The above seemingly simple code is dead-lock prone.  There is no way
> to know what map.get() is actually doing.  It could post the update to
> another thread, waiting for processing to occur(like some kind of
> database transaction threadpool worker).
>
> Basically, if you use synchronized, you have to be *very* careful
> about the methods you call while you have a lock held.  There is no
> guarantee about what locks the called code may be taking.  It might
> even work now, but the library could change in the future to do
> different locking.

This is good information, but... what is the correct way to do it? Can
you give us a couple of refactoring examples?

[Adam Heath-2]

Adrian Crum wrote:
> Adam Heath wrote:
>> public synchronized void fooMethod(Map map) {
>>     // do something
>>     boolean result = map.containsKey("foo");
>>     // do something else
>> }
>>

public void fooMethod(Map map) {
    synchronized (this) {
        // do something
    }
    boolean result = map.containsKey("foo");
    synchronized (this) {
        // do something else
    }
}

>>
>> public class MyMap extends Map {
>>     public synchronized Object get(Object key) {
>>         // do something
>>         fooMethod(this);
>>         // do something else
>>         return keyValue;
>>     }
>> }

public Object get(Object key) {
    synchronized (this) {
        // do something
    }
    fooMethod(this);
    synchronized (this) {
        // do something else
    }
}

>>
>>
>> The above seemingly simple code is dead-lock prone.  There is no way
>> to know what map.get() is actually doing.  It could post the update to
>> another thread, waiting for processing to occur(like some kind of
>> database transaction threadpool worker).
>>
>> Basically, if you use synchronized, you have to be *very* careful
>> about the methods you call while you have a lock held.  There is no
>> guarantee about what locks the called code may be taking.  It might
>> even work now, but the library could change in the future to do
>> different locking.
>
> This is good information, but... what is the correct way to do it? Can
> you give us a couple of refactoring examples?

Basically, it's not that the 2 methods in tandem are broken, it's that
both are broken.  Fixing just one won't solve the actual problem, as
either method could be used with another implementation that doesn't
have correct locking.

Every method needs to be fixed to be very careful when calling outside
code.

Unfortunately, there is no easy fix for these, you have to understand
very deeply how the code in question functions.


>
>