Skip to content

Commit 51751ec

Browse files
mookkiahmookkiah
committed
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.
1 parent f47dc1e commit 51751ec

5 files changed

Lines changed: 127 additions & 75 deletions

File tree

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

Lines changed: 31 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -23,14 +23,15 @@
2323

2424
package com.iluwatar.reader.writer.lock;
2525

26-
import org.slf4j.Logger;
27-
import org.slf4j.LoggerFactory;
28-
2926
import java.util.concurrent.ExecutorService;
3027
import java.util.concurrent.Executors;
28+
import java.util.concurrent.ThreadLocalRandom;
3129
import java.util.concurrent.TimeUnit;
3230
import java.util.stream.IntStream;
3331

32+
import org.slf4j.Logger;
33+
import org.slf4j.LoggerFactory;
34+
3435
/**
3536
*
3637
* In a multiple thread applications, the threads may try to synchronize the shared resources
@@ -62,21 +63,42 @@ public static void main(String[] args) {
6263

6364
ExecutorService executeService = Executors.newFixedThreadPool(10);
6465
ReaderWriterLock lock = new ReaderWriterLock();
65-
66-
// Start 5 readers
66+
67+
// Start writers
6768
IntStream.range(0, 5)
68-
.forEach(i -> executeService.submit(new Reader("Reader " + i, lock.readLock())));
69+
.forEach(i -> executeService.submit(new Writer("Writer " + i, lock.writeLock(),
70+
ThreadLocalRandom.current().nextLong(5000))));
71+
LOGGER.info("Writers added...");
6972

70-
// Start 5 writers
73+
// Start readers
7174
IntStream.range(0, 5)
72-
.forEach(i -> executeService.submit(new Writer("Writer " + i, lock.writeLock())));
75+
.forEach(i -> executeService.submit(new Reader("Reader " + i, lock.readLock(),
76+
ThreadLocalRandom.current().nextLong(10))));
77+
LOGGER.info("Readers added...");
78+
79+
try {
80+
Thread.sleep(5000L);
81+
} catch (InterruptedException e) {
82+
LOGGER.error("Error sleeping before adding more readers", e);
83+
Thread.currentThread().interrupt();
84+
}
85+
86+
// Start readers
87+
IntStream.range(6, 10)
88+
.forEach(i -> executeService.submit(new Reader("Reader " + i, lock.readLock(),
89+
ThreadLocalRandom.current().nextLong(10))));
90+
LOGGER.info("More readers added...");
91+
92+
93+
7394
// In the system console, it can see that the read operations are executed concurrently while
7495
// write operations are exclusive.
7596
executeService.shutdown();
7697
try {
7798
executeService.awaitTermination(5, TimeUnit.SECONDS);
7899
} catch (InterruptedException e) {
79-
LOGGER.error("Error waiting for ExecutorService shutdown");
100+
LOGGER.error("Error waiting for ExecutorService shutdown", e);
101+
Thread.currentThread().interrupt();
80102
}
81103

82104
}

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

Lines changed: 27 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -22,11 +22,11 @@
2222
*/
2323
package com.iluwatar.reader.writer.lock;
2424

25+
import java.util.concurrent.locks.Lock;
26+
2527
import org.slf4j.Logger;
2628
import org.slf4j.LoggerFactory;
2729

28-
import java.util.concurrent.locks.Lock;
29-
3030
/**
3131
* Reader class, read when it acquired the read lock
3232
*/
@@ -37,10 +37,30 @@ public class Reader implements Runnable {
3737
private Lock readLock;
3838

3939
private String name;
40+
41+
private long readingTime;
4042

41-
public Reader(String name, Lock readLock) {
43+
/**
44+
* Create new Reader
45+
*
46+
* @param name - Name of the thread owning the reader
47+
* @param readLock - Lock for this reader
48+
* @param readingTime - amount of time (in milliseconds) for this reader to engage reading
49+
*/
50+
public Reader(String name, Lock readLock, long readingTime) {
4251
this.name = name;
4352
this.readLock = readLock;
53+
this.readingTime = readingTime;
54+
}
55+
56+
/**
57+
* Create new Reader who reads for 250ms
58+
*
59+
* @param name - Name of the thread owning the reader
60+
* @param readLock - Lock for this reader
61+
*/
62+
public Reader(String name, Lock readLock) {
63+
this(name, readLock, 250L);
4464
}
4565

4666
@Override
@@ -49,7 +69,8 @@ public void run() {
4969
try {
5070
read();
5171
} catch (InterruptedException e) {
52-
e.printStackTrace();
72+
LOGGER.info("InterruptedException when reading", e);
73+
Thread.currentThread().interrupt();
5374
} finally {
5475
readLock.unlock();
5576
}
@@ -61,7 +82,7 @@ public void run() {
6182
*/
6283
public void read() throws InterruptedException {
6384
LOGGER.info("{} begin", name);
64-
Thread.sleep(250);
65-
LOGGER.info("{} finish", name);
85+
Thread.sleep(readingTime);
86+
LOGGER.info("{} finish after reading {}ms", name, readingTime);
6687
}
6788
}

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

Lines changed: 41 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -29,13 +29,18 @@
2929
import java.util.concurrent.locks.Lock;
3030
import java.util.concurrent.locks.ReadWriteLock;
3131

32+
import org.slf4j.Logger;
33+
import org.slf4j.LoggerFactory;
34+
3235
/**
3336
* Class responsible for control the access for reader or writer
3437
*
35-
* Allows multiple readers to hold the lock at same time, but if any writer holds the lock then
36-
* readers wait. If reader holds the lock then writer waits. This lock is not fair.
38+
* Allows multiple readers to hold the lock at same time, but if any writer holds the lock then readers wait. If reader
39+
* holds the lock then writer waits. This lock is not fair.
3740
*/
3841
public class ReaderWriterLock implements ReadWriteLock {
42+
43+
private static final Logger LOGGER = LoggerFactory.getLogger(ReaderWriterLock.class);
3944

4045

4146
private Object readerMutex = new Object();
@@ -45,10 +50,10 @@ public class ReaderWriterLock implements ReadWriteLock {
4550
/**
4651
* Global mutex is used to indicate that whether reader or writer gets the lock in the moment.
4752
* <p>
48-
* 1. When it contains the reference of {@link readerLock}, it means that the lock is acquired by
49-
* the reader, another reader can also do the read operation concurrently. <br>
50-
* 2. When it contains the reference of reference of {@link writerLock}, it means that the lock is
51-
* acquired by the writer exclusively, no more reader or writer can get the lock.
53+
* 1. When it contains the reference of {@link readerLock}, it means that the lock is acquired by the reader, another
54+
* reader can also do the read operation concurrently. <br>
55+
* 2. When it contains the reference of reference of {@link writerLock}, it means that the lock is acquired by the
56+
* writer exclusively, no more reader or writer can get the lock.
5257
* <p>
5358
* This is the most important field in this class to control the access for reader/writer.
5459
*/
@@ -74,13 +79,6 @@ private boolean doesWriterOwnThisLock() {
7479
return globalMutex.contains(writerLock);
7580
}
7681

77-
/**
78-
* return true when globalMutex hold the reference of readerLock
79-
*/
80-
private boolean doesReaderOwnThisLock() {
81-
return globalMutex.contains(readerLock);
82-
}
83-
8482
/**
8583
* Nobody get the lock when globalMutex contains nothing
8684
*
@@ -89,43 +87,39 @@ private boolean isLockFree() {
8987
return globalMutex.isEmpty();
9088
}
9189

92-
private static void waitUninterruptibly(Object o) {
93-
try {
94-
o.wait();
95-
} catch (InterruptedException e) {
96-
e.printStackTrace();
97-
}
98-
}
99-
10090
/**
10191
* Reader Lock, can be access for more than one reader concurrently if no writer get the lock
10292
*/
10393
private class ReadLock implements Lock {
10494

10595
@Override
10696
public void lock() {
107-
10897
synchronized (readerMutex) {
109-
11098
currentReaderCount++;
11199
if (currentReaderCount == 1) {
112-
// Try to get the globalMutex lock for the first reader
113-
synchronized (globalMutex) {
114-
while (true) {
115-
// If the no one get the lock or the lock is locked by reader, just set the reference
116-
// to the globalMutex to indicate that the lock is locked by Reader.
117-
if (isLockFree() || doesReaderOwnThisLock()) {
118-
globalMutex.add(this);
119-
break;
120-
} else {
121-
// If lock is acquired by the write, let the thread wait until the writer release
122-
// the lock
123-
waitUninterruptibly(globalMutex);
124-
}
125-
}
126-
}
100+
acquireForReaders();
101+
}
102+
}
103+
}
127104

105+
/**
106+
* Acquire the globalMutex lock on behalf of current and future concurrent readers. Make sure no writers currently
107+
* owns the lock.
108+
*/
109+
private void acquireForReaders() {
110+
// Try to get the globalMutex lock for the first reader
111+
synchronized (globalMutex) {
112+
// If the no one get the lock or the lock is locked by reader, just set the reference
113+
// to the globalMutex to indicate that the lock is locked by Reader.
114+
while (doesWriterOwnThisLock()) {
115+
try {
116+
globalMutex.wait();
117+
} catch (InterruptedException e) {
118+
LOGGER.info("InterruptedException while waiting for globalMutex in acquireForReaders", e);
119+
Thread.currentThread().interrupt();
120+
}
128121
}
122+
globalMutex.add(this);
129123
}
130124
}
131125

@@ -179,23 +173,17 @@ public void lock() {
179173

180174
synchronized (globalMutex) {
181175

182-
while (true) {
183-
// When there is no one acquired the lock, just put the writeLock reference to the
184-
// globalMutex to indicate that the lock is acquired by one writer.
185-
// It is ensure that writer can only get the lock when no reader/writer acquired the lock.
186-
if (isLockFree()) {
187-
globalMutex.add(this);
188-
break;
189-
} else if (doesWriterOwnThisLock()) {
190-
// Wait when other writer get the lock
191-
waitUninterruptibly(globalMutex);
192-
} else if (doesReaderOwnThisLock()) {
193-
// Wait when other reader get the lock
194-
waitUninterruptibly(globalMutex);
195-
} else {
196-
throw new AssertionError("it should never reach here");
176+
// Wait until the lock is free.
177+
while (!isLockFree()) {
178+
try {
179+
globalMutex.wait();
180+
} catch (InterruptedException e) {
181+
LOGGER.info("InterruptedException while waiting for globalMutex to begin writing", e);
182+
Thread.currentThread().interrupt();
197183
}
198184
}
185+
// When the lock is free, acquire it by placing an entry in globalMutex
186+
globalMutex.add(this);
199187
}
200188
}
201189

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

Lines changed: 27 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -22,11 +22,11 @@
2222
*/
2323
package com.iluwatar.reader.writer.lock;
2424

25+
import java.util.concurrent.locks.Lock;
26+
2527
import org.slf4j.Logger;
2628
import org.slf4j.LoggerFactory;
2729

28-
import java.util.concurrent.locks.Lock;
29-
3030
/**
3131
* Writer class, write when it acquired the write lock
3232
*/
@@ -37,10 +37,30 @@ public class Writer implements Runnable {
3737
private Lock writeLock;
3838

3939
private String name;
40+
41+
private long writingTime;
4042

43+
/**
44+
* Create new Writer who writes for 250ms
45+
*
46+
* @param name - Name of the thread owning the writer
47+
* @param writeLock - Lock for this writer
48+
*/
4149
public Writer(String name, Lock writeLock) {
50+
this(name, writeLock, 250L);
51+
}
52+
53+
/**
54+
* Create new Writer
55+
*
56+
* @param name - Name of the thread owning the writer
57+
* @param writeLock - Lock for this writer
58+
* @param writingTime - amount of time (in milliseconds) for this reader to engage writing
59+
*/
60+
public Writer(String name, Lock writeLock, long writingTime) {
4261
this.name = name;
4362
this.writeLock = writeLock;
63+
this.writingTime = writingTime;
4464
}
4565

4666

@@ -50,18 +70,19 @@ public void run() {
5070
try {
5171
write();
5272
} catch (InterruptedException e) {
53-
e.printStackTrace();
73+
LOGGER.info("InterruptedException when writing", e);
74+
Thread.currentThread().interrupt();
5475
} finally {
5576
writeLock.unlock();
5677
}
5778
}
58-
79+
5980
/**
6081
* Simulate the write operation
6182
*/
6283
public void write() throws InterruptedException {
6384
LOGGER.info("{} begin", name);
64-
Thread.sleep(250);
65-
LOGGER.info("{} finish", name);
85+
Thread.sleep(writingTime);
86+
LOGGER.info("{} finished after writing {}ms", name, writingTime);
6687
}
6788
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,6 @@ protected void append(ILoggingEvent eventObject) {
5252
}
5353

5454
public boolean logContains(String message) {
55-
return log.stream().anyMatch(event -> event.getFormattedMessage().equals(message));
55+
return log.stream().anyMatch(event -> event.getFormattedMessage().contains(message));
5656
}
5757
}

0 commit comments

Comments
 (0)