1

We are writing a concurrent multimap to store multiple values for the same key in a multi threaded application. We have extended Guava ForwardingMultimap to do the same. Put and remove methods acquire a write lock on a key and release it at the end. Similarly get acquire a read lock on key and release it at the end. I have tested this by having multiple thread in local system and it works fine.

Now the problem is with the clear method, if one thread is doing put/remove and other thread does clear then the inconsistency happened. To handle this problem, in clear method we decided to put a writelock on public key and release it at the end. Similarly put and remove methods were improved to have a read lock on public key and release it at the end. It is something like below.

IResourceLockManager mLocks = new ResourceLockManager();
private final String mPublicKey = "$public";
private final int mLockTimeOut = 0;

Put/Remove Operation

public boolean put(Object aKey, Object aValue) {
   mLocks.acquireReadLock(mPublicKey, mLockTimeOut);
   mLocks.acquireWriteLock(aKey, mLockTimeOut);
   boolean result = false;
   try {
      result = delegate().put(aKey, aValue);
   } finally {
      mLocks.releaseWriteLock(aKey);
      mLocks.releaseReadLock(mPublicKey);
   }
   return result;
}

Clear Operation

public void clear() {
   mLocks.acquireWriteLock(mPublicKey, mLockTimeOut);
   try {
      delegate().clear();
   } finally {
      mLocks.releaseWriteLock(mPublicKey); 
   }
}

ResourceLockManager Code

Striped<ReadWriteLock> lockCache = Striped.lazyWeakReadWriteLock(DEFAULT_PARTITIONS);

acquireReadLock

lockCache.get(aKey).readLock().lock()

releaseReadLock

lockCache.get(aKey).readLock().unlock();

acquireWriteLock

lockCache.get(aKey).writeLock().lock()

releaseWriteLock

lockCache.get(aKey).writeLock().unlock();

After implementing read/write lock on public key to handle clear method case, I started facing below issue:

Exception in thread "pool-5-thread-10" java.lang.IllegalMonitorStateException: attempt to unlock read lock, not locked by current thread
    at java.util.concurrent.locks.ReentrantReadWriteLock$Sync.unmatchedUnlockException(ReentrantReadWriteLock.java:444)
    at java.util.concurrent.locks.ReentrantReadWriteLock$Sync.tryReleaseShared(ReentrantReadWriteLock.java:428)
    at java.util.concurrent.locks.AbstractQueuedSynchronizer.releaseShared(AbstractQueuedSynchronizer.java:1341)
    at java.util.concurrent.locks.ReentrantReadWriteLock$ReadLock.unlock(ReentrantReadWriteLock.java:881)
    at com.google.common.util.concurrent.ForwardingLock.unlock(ForwardingLock.java:48)
    at org.multimap.rt.locks.ResourceLockManager.releaseReadLock(ResourceLockManager.java:97)
    at org.multimap.rt.util.ConcurrentMultiMap.put(ConcurrentMultiMap.java:81)
    at org.multimap.rt.util.OneShotTask.run(ConcurrentMultiMapTest.java:233)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
    at java.lang.Thread.run(Thread.java:748)

The above issue is not just coming for put, it is sometimes coming during remove as well and it is not coming always. It is like it is coming once in every 3-4 runs.

Another Observation - To test concurrency, if we create 44 or less thread, then I am not seeing this issue in my local system. As soon as I make it 45 or more then I started seeing the exception.

Unit Test Code:

   public void TestConcurrentMultiMap() throws InterruptedException {
      ExecutorService executor = Executors.newFixedThreadPool(45);
      for (int i = 0; i < 45; i++) {
         int num = (int) Math.round(Math.floor(Math.random() * 3));
         int oprn = (int) Math.round(Math.random());
         executor.execute(new OneShotTask(String.valueOf(num), myMap1, oprn));
      }
      executor.shutdown();
      executor.awaitTermination(1000L, TimeUnit.MILLISECONDS);
   }

class OneShotTask implements Runnable {
   String str;
   Multimap mMap;
   int mOprn;

   OneShotTask(String s, Multimap aMap, int oprn) {
      str = s;
      mOprn = oprn;
      mMap = aMap;
   }
   public void run() {
      try {
         Thread.sleep(100);
       } catch (InterruptedException e) {
         throw new RuntimeException(e);
       }
       if(mOprn == 0) {
          mMap.put("key", "value" + str );
       } else { 
          mMap.remove("key", "value" + str ); 
      }
   }
}
arun
  • 388
  • 2
  • 17
  • I doubt that anybody will be able to tell you anything more than what the stack trace tells you if you don't show the source code in your question. What do the `ConcurrentMultiMap.put()`, `remove()`, `get()`, and `clear()` operations look like? What does the `AeResourceLockManager` class look like? What does the `OneShotTask.run()` method do? – Solomon Slow Nov 29 '22 at 22:48
  • @SolomonSlow - Added OneShotTask class code snippet. This is created to do concurrent test. Please look at ResourceLockManager code section, you will find what this class does. ConcurrentMultiMap operations have also covered in plain text what they does. The actual problem is if I provide the thread count less than 45 then i don't see any issue. As soon as I make it to 45 or more then i started seeing above issue. – arun Nov 30 '22 at 06:07
  • Re, "the actual problem is...45." That's not the problem. That's a symptom. The problem is, there is something wrong with your code—code that you still haven't showed to anybody. Knowing that it takes 45 threads to manifest the problem, and not 44 or 17 or 110 is not going to help anybody to figure out what you did wrong. We need to see the code. So, Now we know the order in which `OneShotTask` calls `mMap.put()` and `remove()`, but we still don't know what those methods do. And those methods call in to your `ResourceLockManager`, and we still don't know what that does either. – Solomon Slow Nov 30 '22 at 14:10
  • @SolomonSlow Updated the code for Put and clear. ResourceLockManager code is already available, there are 4 methods - acquireReadLock, releaseReadLock, acquireWriteLock and releaseWriteLock – arun Dec 01 '22 at 07:30
  • This whole design seems off. Why do you think it's safe to run concurrent `put`s on different partitions? Why do you assume it's safe to even read from one partition while writing to another? – shmosel Dec 01 '22 at 07:53
  • Also, how does the timeout work? If it throws an exception, you can have a scenario where the read lock is acquired and never released. You probably want a nested try/finally block before the write. – shmosel Dec 01 '22 at 07:55
  • Instead of using striped locks over a single map, how about partitioning the data into multiple maps so you can be sure they don't interfere with each other? You can do aggregate operations (`size()`, `clear()`) under a write lock and key-based operations (`put()`, `get()`) under a read lock. – shmosel Dec 01 '22 at 08:06
  • @SolomonSlow We want other threads to run in parallel for the different key without blocking them, hence we moved to this design. Regarding timeout, as I am passing 0 there so it won't execute timeout related lock code. Didn't get your last comment on partitioning data into multiple maps. Our keys are dynamic, there will be multiple put/remove/get operations on this map. If we create multiple maps we end up creating so many maps. Do you have any sample code to show what you said? – arun Dec 02 '22 at 06:20

1 Answers1

0

Looks like there is a bug in Striped.lazyWeakReadWriteLock(). By changing the initialization from Striped.lazyWeakReadWriteLock to Striped.readWriteLock(), I am unable to reproduce it.

More details here - https://github.com/google/guava/issues/2477

arun
  • 388
  • 2
  • 17