iluwatar#587 SonarQube reports bugs reader-writer-lock and refactor

Keeping wait() calls with in synchronized block closely to adhere
SonarQube rules.
 
Avoid nested synchronized block by extracting method.

Added writing and reading time to simulate testing to ensure 
1) writers are waiting for globalMutex to be empty 
2) readers to confirm there is no writers.

Author: mookkiah

reader-writer-lock/src/main/java/com/iluwatar/reader/writer/lock/App.java

@@ -23,14 +23,15 @@
 
 package com.iluwatar.reader.writer.lock;
 
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
-
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
+import java.util.concurrent.ThreadLocalRandom;
 import java.util.concurrent.TimeUnit;
 import java.util.stream.IntStream;
 
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
 /**
  * 
  * In a multiple thread applications, the threads may try to synchronize the shared resources
@@ -62,21 +63,42 @@ public static void main(String[] args) {
 
     ExecutorService executeService = Executors.newFixedThreadPool(10);
     ReaderWriterLock lock = new ReaderWriterLock();
-
-    // Start 5 readers
+    
+    // Start writers
     IntStream.range(0, 5)
-        .forEach(i -> executeService.submit(new Reader("Reader " + i, lock.readLock())));
+        .forEach(i -> executeService.submit(new Writer("Writer " + i, lock.writeLock(), 
+            ThreadLocalRandom.current().nextLong(5000))));
+    LOGGER.info("Writers added...");
 
-    // Start 5 writers
+    // Start readers
     IntStream.range(0, 5)
-        .forEach(i -> executeService.submit(new Writer("Writer " + i, lock.writeLock())));
+        .forEach(i -> executeService.submit(new Reader("Reader " + i, lock.readLock(), 
+            ThreadLocalRandom.current().nextLong(10))));
+    LOGGER.info("Readers added...");
+    
+    try {
+      Thread.sleep(5000L);
+    } catch (InterruptedException e) {
+      LOGGER.error("Error sleeping before adding more readers", e);
+      Thread.currentThread().interrupt();
+    }
+
+    // Start readers
+    IntStream.range(6, 10)
+        .forEach(i -> executeService.submit(new Reader("Reader " + i, lock.readLock(), 
+            ThreadLocalRandom.current().nextLong(10))));
+    LOGGER.info("More readers added...");
+    
+    
+
     // In the system console, it can see that the read operations are executed concurrently while
     // write operations are exclusive.
     executeService.shutdown();
     try {
       executeService.awaitTermination(5, TimeUnit.SECONDS);
     } catch (InterruptedException e) {
-      LOGGER.error("Error waiting for ExecutorService shutdown");
+      LOGGER.error("Error waiting for ExecutorService shutdown", e);
+      Thread.currentThread().interrupt();
     }
 
   }

reader-writer-lock/src/main/java/com/iluwatar/reader/writer/lock/Reader.java

@@ -22,11 +22,11 @@
  */
 package com.iluwatar.reader.writer.lock;
 
+import java.util.concurrent.locks.Lock;
+
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-import java.util.concurrent.locks.Lock;
-
 /**
  * Reader class, read when it acquired the read lock
  */
@@ -37,10 +37,30 @@ public class Reader implements Runnable {
   private Lock readLock;
 
   private String name;
+  
+  private long readingTime;
 
-  public Reader(String name, Lock readLock) {
+  /**
+   * Create new Reader
+   * 
+   * @param name - Name of the thread owning the reader
+   * @param readLock - Lock for this reader
+   * @param readingTime - amount of time (in milliseconds) for this reader to engage reading
+   */
+  public Reader(String name, Lock readLock, long readingTime) {
     this.name = name;
     this.readLock = readLock;
+    this.readingTime = readingTime;
+  }
+  
+  /**
+   * Create new Reader who reads for 250ms
+   * 
+   * @param name - Name of the thread owning the reader
+   * @param readLock - Lock for this reader
+   */
+  public Reader(String name, Lock readLock) {
+    this(name, readLock, 250L);
   }
 
   @Override
@@ -49,7 +69,8 @@ public void run() {
     try {
       read();
     } catch (InterruptedException e) {
-      e.printStackTrace();
+      LOGGER.info("InterruptedException when reading", e);
+      Thread.currentThread().interrupt();
     } finally {
       readLock.unlock();
     }
@@ -61,7 +82,7 @@ public void run() {
    */
   public void read() throws InterruptedException {
     LOGGER.info("{} begin", name);
-    Thread.sleep(250);
-    LOGGER.info("{} finish", name);
+    Thread.sleep(readingTime);
+    LOGGER.info("{} finish after reading {}ms", name, readingTime);
   }
 }

reader-writer-lock/src/main/java/com/iluwatar/reader/writer/lock/ReaderWriterLock.java

@@ -29,13 +29,18 @@
 import java.util.concurrent.locks.Lock;
 import java.util.concurrent.locks.ReadWriteLock;
 
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
 /**
  * Class responsible for control the access for reader or writer
  * 
- * Allows multiple readers to hold the lock at same time, but if any writer holds the lock then
- * readers wait. If reader holds the lock then writer waits. This lock is not fair.
+ * Allows multiple readers to hold the lock at same time, but if any writer holds the lock then readers wait. If reader
+ * holds the lock then writer waits. This lock is not fair.
  */
 public class ReaderWriterLock implements ReadWriteLock {
+  
+  private static final Logger LOGGER = LoggerFactory.getLogger(ReaderWriterLock.class);
 
 
   private Object readerMutex = new Object();
@@ -45,10 +50,10 @@ public class ReaderWriterLock implements ReadWriteLock {
   /**
    * Global mutex is used to indicate that whether reader or writer gets the lock in the moment.
    * <p>
-   * 1. When it contains the reference of {@link readerLock}, it means that the lock is acquired by
-   * the reader, another reader can also do the read operation concurrently. <br>
-   * 2. When it contains the reference of reference of {@link writerLock}, it means that the lock is
-   * acquired by the writer exclusively, no more reader or writer can get the lock.
+   * 1. When it contains the reference of {@link readerLock}, it means that the lock is acquired by the reader, another
+   * reader can also do the read operation concurrently. <br>
+   * 2. When it contains the reference of reference of {@link writerLock}, it means that the lock is acquired by the
+   * writer exclusively, no more reader or writer can get the lock.
    * <p>
    * This is the most important field in this class to control the access for reader/writer.
    */
@@ -74,13 +79,6 @@ private boolean doesWriterOwnThisLock() {
     return globalMutex.contains(writerLock);
   }
 
-  /**
-   * return true when globalMutex hold the reference of readerLock
-   */
-  private boolean doesReaderOwnThisLock() {
-    return globalMutex.contains(readerLock);
-  }
-
   /**
    * Nobody get the lock when globalMutex contains nothing
    * 
@@ -89,43 +87,39 @@ private boolean isLockFree() {
     return globalMutex.isEmpty();
   }
 
-  private static void waitUninterruptibly(Object o) {
-    try {
-      o.wait();
-    } catch (InterruptedException e) {
-      e.printStackTrace();
-    }
-  }
-
   /**
    * Reader Lock, can be access for more than one reader concurrently if no writer get the lock
    */
   private class ReadLock implements Lock {
 
     @Override
     public void lock() {
-
       synchronized (readerMutex) {
-
         currentReaderCount++;
         if (currentReaderCount == 1) {
-          // Try to get the globalMutex lock for the first reader
-          synchronized (globalMutex) {
-            while (true) {
-              // If the no one get the lock or the lock is locked by reader, just set the reference
-              // to the globalMutex to indicate that the lock is locked by Reader.
-              if (isLockFree() || doesReaderOwnThisLock()) {
-                globalMutex.add(this);
-                break;
-              } else {
-                // If lock is acquired by the write, let the thread wait until the writer release
-                // the lock
-                waitUninterruptibly(globalMutex);
-              }
-            }
-          }
+          acquireForReaders();
+        }
+      }
+    }
 
+    /**
+     * Acquire the globalMutex lock on behalf of current and future concurrent readers. Make sure no writers currently
+     * owns the lock.
+     */
+    private void acquireForReaders() {
+      // Try to get the globalMutex lock for the first reader
+      synchronized (globalMutex) {
+        // If the no one get the lock or the lock is locked by reader, just set the reference
+        // to the globalMutex to indicate that the lock is locked by Reader.
+        while (doesWriterOwnThisLock()) {
+          try {
+            globalMutex.wait();
+          } catch (InterruptedException e) {
+            LOGGER.info("InterruptedException while waiting for globalMutex in acquireForReaders", e);
+            Thread.currentThread().interrupt();
+          }
         }
+        globalMutex.add(this);
       }
     }
 
@@ -179,23 +173,17 @@ public void lock() {
 
       synchronized (globalMutex) {
 
-        while (true) {
-          // When there is no one acquired the lock, just put the writeLock reference to the
-          // globalMutex to indicate that the lock is acquired by one writer.
-          // It is ensure that writer can only get the lock when no reader/writer acquired the lock.
-          if (isLockFree()) {
-            globalMutex.add(this);
-            break;
-          } else if (doesWriterOwnThisLock()) {
-            // Wait when other writer get the lock
-            waitUninterruptibly(globalMutex);
-          } else if (doesReaderOwnThisLock()) {
-            // Wait when other reader get the lock
-            waitUninterruptibly(globalMutex);
-          } else {
-            throw new AssertionError("it should never reach here");
+        // Wait until the lock is free.
+        while (!isLockFree()) {
+          try {
+            globalMutex.wait();
+          } catch (InterruptedException e) {
+            LOGGER.info("InterruptedException while waiting for globalMutex to begin writing", e);
+            Thread.currentThread().interrupt();
           }
         }
+        // When the lock is free, acquire it by placing an entry in globalMutex
+        globalMutex.add(this);
       }
     }
 

reader-writer-lock/src/main/java/com/iluwatar/reader/writer/lock/Writer.java

@@ -22,11 +22,11 @@
  */
 package com.iluwatar.reader.writer.lock;
 
+import java.util.concurrent.locks.Lock;
+
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-import java.util.concurrent.locks.Lock;
-
 /**
  * Writer class, write when it acquired the write lock
  */
@@ -37,10 +37,30 @@ public class Writer implements Runnable {
   private Lock writeLock;
 
   private String name;
+  
+  private long writingTime;
 
+  /**
+   * Create new Writer who writes for 250ms
+   * 
+   * @param name - Name of the thread owning the writer
+   * @param writeLock - Lock for this writer
+   */
   public Writer(String name, Lock writeLock) {
+    this(name, writeLock, 250L);
+  }
+  
+  /**
+   * Create new Writer
+   * 
+   * @param name - Name of the thread owning the writer
+   * @param writeLock - Lock for this writer
+   * @param writingTime - amount of time (in milliseconds) for this reader to engage writing
+   */
+  public Writer(String name, Lock writeLock, long writingTime) {
     this.name = name;
     this.writeLock = writeLock;
+    this.writingTime = writingTime;
   }
 
 
@@ -50,18 +70,19 @@ public void run() {
     try {
       write();
     } catch (InterruptedException e) {
-      e.printStackTrace();
+      LOGGER.info("InterruptedException when writing", e);
+      Thread.currentThread().interrupt();
     } finally {
       writeLock.unlock();
     }
   }
-
+  
   /**
    * Simulate the write operation
    */
   public void write() throws InterruptedException {
     LOGGER.info("{} begin", name);
-    Thread.sleep(250);
-    LOGGER.info("{} finish", name);
+    Thread.sleep(writingTime);
+    LOGGER.info("{} finished after writing {}ms", name, writingTime);
   }
 }

reader-writer-lock/src/test/java/com/iluwatar/reader/writer/lock/utils/InMemoryAppender.java

@@ -52,6 +52,6 @@ protected void append(ILoggingEvent eventObject) {
   }
 
   public boolean logContains(String message) {
-    return log.stream().anyMatch(event -> event.getFormattedMessage().equals(message));
+    return log.stream().anyMatch(event -> event.getFormattedMessage().contains(message));
   }
 }