How to refactor a synchronization block based on string

Question

I am refactoring a legacy project and I have found a code that uses String for synchronization (Actually it's a Cache implementation).

public void method (String key, ...) {

 synchronized(key) {
   ....
  }
}

There is only one block which synchronize on String object key.

I am wondering which is the best way for fixing/Refactoring the code?

Any ideas?

The first thing to do is to clarify whether there are other `synchronized` blocks, supposed to synchronize on the same object. — Holger, Oct 29 '15 at 22:58
Have a look at [Synchronizing on an ID](http://illegalargumentexception.blogspot.fi/2008/04/java-synchronizing-on-transient-id.html). — Mick Mnemonic, Oct 29 '15 at 23:02
@MickMnemonic the implementation adds a synchronization level to get the lock. Isn't that heavy? — Master Mind, Oct 29 '15 at 23:08
Might be; it really depends on your application. If you want high throughput, consider using a [Guava Cache](https://github.com/google/guava/wiki/CachesExplained). An example implementation is given in [Using Guava for high performance thread-safe caching](http://stackoverflow.com/questions/11124856/using-guava-for-high-performance-thread-safe-caching). — Mick Mnemonic, Oct 29 '15 at 23:12
Yeah it's sure that's a better option in the mean I have to live the old fat code — Master Mind, Oct 29 '15 at 23:13
You have not described any problem. All you have described are facts. Why do you want to rewrite this code? What are the perceived problems? BTW what you want to do is NOT a refactoring. You want to rewrite. — JnRouvignac, Oct 30 '15 at 06:46
@JnRouvignac I am ok it's not refactoring it's potential bug fix. Visit https://www.securecoding.cert.org/confluence/display/java/LCK01-J.+Do+not+synchronize+on+objects+that+may+be+reused — Master Mind, Oct 30 '15 at 10:29
See also https://stackoverflow.com/questions/133988/synchronizing-on-string-objects-in-java — Raedwald, Aug 16 '18 at 10:28

score 3 · Accepted Answer · answered Oct 29 '15 at 23:08

One way to do it is the following pattern:

private final ConcurrentHashMap<String, Object> lockMap = new ConcurrentHashMap<>();
public void method(String key, ...) {
    synchronized(getLock(key)) {
        ....
    }
}
protected Object getLock(String key) {
    Object newLock = new Object(), lock = lockMap.putIfAbsent(key, newLock);
    return lock == null? newLock: lock;
}

Note that this is a proven pattern as the parallel class loaders, introduced with Java 7, use it as well.

With Java 8, you can simplify the code:

protected Object getLock(String key) {
    return lockMap.computeIfAbsent(key, x->new Object());
}

How to refactor a synchronization block based on string

1 Answers1