7

I want to synchronize method calls on basis some id i.e. something like a concurrency Decorator of a given object instance.
For example:
All threads which call the method with param "id1", should execute serially to one another.
All of the rest, which call the method with different argument, say "id2", should execute in parallel to the threads which call the method with param "id1", but again serially to each other.

So in my mind this can be implemented by having a lock (http://docs.oracle.com/javase/6/docs/api/java/util/concurrent/locks/ReentrantLock.html) instance per such method param. Each time the method is called with the param, the lock instance corresponding to the specific param value (e.g. "id1") would be looked up and the current thread would try to obtain the lock.

Speaking in code:

public class ConcurrentPolicyWrapperImpl implements Foo {

private Foo delegate;

/**
* Holds the monitor objects used for synchronization.
*/
private Map<String, Lock> concurrentPolicyMap = Collections.synchronizedMap(new HashMap<String, Lock>());

/**
* Here we decorate the call to the wrapped instance with a synchronization policy.
*/
@Override
public Object callFooDelegateMethod (String id) {
        Lock lock = getLock(id);
        lock.lock();
        try {
            return delegate.delegateMethod(id);
        } finally {
            lock.unlock();
        }
}


protected Lock getLock(String id) {
        Lock lock = concurrentPolicyMap.get(id);

        if (lock == null) {
            lock = createLock();
            concurrentPolicyMap.put(id, lock);

        }
        return lock;
    }

}

protected Lock createLock() {
        return new ReentrantLock();
    }

It seems that this works - I did some performance testing with jmeter and so on. Still, as we all know concurrency in Java is a tricky thing, I decided to ask for your opinion here.

I can't stop thinking that there could be a better way to accomplish this. For example by using one of the BlockingQueue implementations. What do you think?

I also can't really decide for sure if there is a potential synchronization problem with getting the lock i.e. the protected Lock getLock(String id) method. I am using a synchronized collection, but is that enough? I.e. shouldn't it be something like the following instead of what I currently have:

protected Lock getLock(String id) {
  synchronized(concurrentPolicyMap) {
    Lock lock = concurrentPolicyMap.get(id);

    if (lock == null) {
      lock = createLock();
      concurrentPolicyMap.put(id, lock);

    }
    return lock;
  }

}

So what do you guys think?

Svilen
  • 1,377
  • 1
  • 16
  • 23

2 Answers2

4

Lock creation issues aside, the pattern is OK except that you may have an unbounded number of locks. Generally people avoid this by creating/using a Striped lock. There is a good/simple implementation in the guava library.

Application area of lock-striping

How to acquire a lock by a key

http://docs.guava-libraries.googlecode.com/git/javadoc/com/google/common/util/concurrent/Striped.html

Example code using guava implementation:

private Striped<Lock> STRIPPED_LOCK = Striped.lock(64);

public static void doActualWork(int id) throws InterruptedException {
    try {
        STRIPPED_LOCK.get(id).lock();
        ...
    } finally {
        STRIPPED_LOCK.get(id).unlock();
    }
}
Community
  • 1
  • 1
Keith
  • 4,144
  • 1
  • 19
  • 14
  • You are saying "Lock creation issues aside"... Do you spot any or do you mean that you are not commenting on it? – Svilen May 30 '13 at 15:35
  • Looking at that issue more closely now-- yeah, you would definitley need the synchronized block in your getLock() method. As you have it in the second example. Otherwise, image (a) 5 threads call getLock() at the same time, for the same ID; (b) they all find the lock null; (c) they all create and return a new lock instance. Fail. The synchronization block in your second bit of code avoids that. – Keith May 30 '13 at 15:46
  • Thank you, Keith. Btw, do you perhaps have any idea how to do the similar thing but with executors/tasks. I.e. have a single ThreadPoolExecutor but partition task execution by a key. For example tasks for key "id1" will run in serial fashion to each other, while at the same time in parallel to all other tasks for keys "id2", "id3", etc... – Svilen May 31 '13 at 15:02
  • To answer my own comment... This is a nice article on this topic: http://www.javaspecialists.eu/archive/Issue206.html Mentioned in this very same article there are some hints on this in the Executor javadocs i.e. SerialExecutor. – Svilen Jun 05 '13 at 13:10
  • @Keith Are you absolutely sure about your implementation since you do not hold a strong reference to the lock object but instead refer to `STRIPED_LOCK` again in the finally block? I ask since using `lazyWeakLock` implies that the locks can be reclaimed when not strongly referenced (seemingly regardless of its lock status). As a result I think that your example code may not be thread safe. – Trevor Freeman Feb 19 '14 at 21:37
  • @increment1 Good catch. I updated the example to use 'Striped.lock(int)' method rather than 'Striped.lazyWeakLock(int)' since weak references are not really relevant to what is being demonstrated here. – Keith Mar 09 '14 at 18:16
1

Though I would personally prefer Guava's Striped<Lock> approach suggested by Keith, just for discussion & completeness, I'd like to point out that using a Dynamic Proxy, or the more generic AOP (Aspect Oriented Programming), is one approach.

So we would define an IStripedConcurrencyAware interface that would serve as the "something like a concurrency Decorator" that you desire, and the Dynamic Proxy / AOP method hijacking based on this interface would de-multiplex the method call into the appropriate Executor / Thread.

I personally dislike AOP (or most of Spring, for that matter) because it breaks the what-you-see-is-what-you-get simplicity of Core Java, but YMMV.

vijucat
  • 2,059
  • 1
  • 15
  • 17