gaspersic/java-design-patterns
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

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

Browse Source 
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.
This commit is contained in:
mahendran.mookkiah 2017-08-18 20:44:28 -04:00
parent f47dc1eb7c
commit 51751ec821
5 changed files with 128 additions and 76 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -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 class App {
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();
}
}

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

@ -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 class Reader implements Runnable {
try {
read();
} catch (InterruptedException e) {
e.printStackTrace();
LOGGER.info("InterruptedException when reading", e);
Thread.currentThread().interrupt();
} finally {
readLock.unlock();
}
@ -61,7 +82,7 @@ public class Reader implements Runnable {
*/
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);
}
}

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

@ -29,13 +29,18 @@ import java.util.concurrent.locks.Condition;
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 @@ public class ReaderWriterLock implements ReadWriteLock {
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,14 +87,6 @@ public class ReaderWriterLock implements ReadWriteLock {
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
*/
@ -104,31 +94,35 @@ public class ReaderWriterLock implements ReadWriteLock {
@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);
}
}
@Override
public void unlock() {
@ -179,23 +173,17 @@ public class ReaderWriterLock implements ReadWriteLock {
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);
}
}

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

@ -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 class Writer implements Runnable {
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);
}
}

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

@ -52,6 +52,6 @@ public class InMemoryAppender extends AppenderBase<ILoggingEvent> {
}
public boolean logContains(String message) {
return log.stream().anyMatch(event -> event.getFormattedMessage().equals(message));
return log.stream().anyMatch(event -> event.getFormattedMessage().contains(message));
}
}