sarveswaran-m / blockingmap4j Goto Github PK

Openlogos awards logs to open-source projects. Each project may claim a logo by creating an issue. The claim that receives the maximum number of upvotes gets the logo. I have created a claim for blockingmap here. Pls upvote the claim.

Originally raised by @kuniss @ https://sourceforge.net/p/blockingmapforj/discussion/874608/thread/646e5523/?limit=25#5cfd

Hi Sarveswaran,
I very appreciate your work!

I have done some tests and encountered a problem on “clear”. I assume that the clear method does not interrupt all threads already waiting on a “take” call.

Here is my test which fails with a test case time out (AutoResetEvent behaves like the same named class in the .NET framework):

public class Test {
    private static ExecutorService executor;

   @BeforeClass
   public static void startExecutor() {
    executor = Executors.newFixedThreadPool(2);
   }

   @AfterClass
   public static void stopExecutor() {
          executor.shutdown();
   }

   private static final int FAIL_TIMEOUT = 2000;
   private volatile String t;

   @Test(timeout=2*FAIL_TIMEOUT+1000)
   public final void testClearWithRunningTake() {
     final BlockingHashMap<String, String> q = new BlockingHashMap<String, String>();
     t = null;

     final AutoResetEvent are = new AutoResetEvent(false);
     executor.execute(new Runnable() {
       @Override
       public void run() {
          are.setSignaled();
          try {
              t = q.take("some non existent key", Integer.MAX_VALUE, TimeUnit.MILLISECONDS);
              fail("interrupted exception expected due to call to clear");
          } catch (InterruptedException e) {
              // expected exception
          }
              are.setSignaled();
          }
     });

     assertTrue(are.waitOne(FAIL_TIMEOUT)); // wait for background tread was started
     sleep(500); // give background thread some more time to ensure take is really waiting
     q.clear();
     assertTrue(are.waitOne(FAIL_TIMEOUT)); // wait on background thread is finished
     assertNull(t);
   }
    
}

I also took a look into the “clear” code and I assume that the lock is blocking when the waiting threads had already acquired the lock. See my notes below in the code:

public void clear() {
    //clear the map only if it has not been cleared yet
    if (!cleared.getAndSet(true)) {
        primaryMapWriteLock.lock(); // DK: the lock comes too early as it may be locked already at a “take” call
        try {
            for (Thread thread : blockedThreadsMap.keySet()) {
                thread.interrupt();
            }
            primaryMap.clear();
        } finally {
            primaryMapWriteLock.unlock();
        }
    }
 }

May be the following is more sufficient (I did not test it yet):

public void clear() {
    //clear the map only if it has not been cleared yet
    if (!cleared.getAndSet(true)) {
        for (Thread thread : blockedThreadsMap.keySet()) { // DK: This will unblock the blocked threads waiting in “take” calls AND one of them at the end will unlock primaryMapWriteLock 
            thread.interrupt();
        }
        primaryMapWriteLock.lock(); // DK: when all threads are interrupted this gets unlocked and this call will succeed
        try {
            primaryMap.clear();
        } finally {
            primaryMapWriteLock.unlock();
        }
    }
}