Skip to content

Commit

Permalink
[netty#2675] Replace synchronization performed on util.concurrent ins…
Browse files Browse the repository at this point in the history
…tance in TrafficCounter

Motivation:

Message from FindBugs:
This method performs synchronization an object that is an instance of a class from the java.util.concurrent package (or its subclasses). Instances of these classes have their own concurrency control mechanisms that are orthogonal to the synchronization provided by the Java keyword synchronized. For example, synchronizing on an AtomicBoolean will not prevent other threads from modifying the AtomicBoolean.
Such code may be correct, but should be carefully reviewed and documented, and may confuse people who have to maintain the code at a later date.

Modification:

Use synchronized(this)

Result:

Less confusing code
  • Loading branch information
Norman Maurer committed Jul 21, 2014
1 parent 6d0233d commit e258919
Showing 1 changed file with 32 additions and 38 deletions.
70 changes: 32 additions & 38 deletions handler/src/main/java/io/netty/handler/traffic/TrafficCounter.java
Original file line number Diff line number Diff line change
Expand Up @@ -163,37 +163,33 @@ public void run() {
/**
* Start the monitoring process
*/
public void start() {
synchronized (lastTime) {
if (monitorActive.get()) {
return;
}
lastTime.set(System.currentTimeMillis());
if (checkInterval.get() > 0) {
monitorActive.set(true);
monitor = new TrafficMonitoringTask(trafficShapingHandler, this);
scheduledFuture =
executor.schedule(monitor, checkInterval.get(), TimeUnit.MILLISECONDS);
}
public synchronized void start() {
if (monitorActive.get()) {
return;
}
lastTime.set(System.currentTimeMillis());
if (checkInterval.get() > 0) {
monitorActive.set(true);
monitor = new TrafficMonitoringTask(trafficShapingHandler, this);
scheduledFuture =
executor.schedule(monitor, checkInterval.get(), TimeUnit.MILLISECONDS);
}
}

/**
* Stop the monitoring process
*/
public void stop() {
synchronized (lastTime) {
if (!monitorActive.get()) {
return;
}
monitorActive.set(false);
resetAccounting(System.currentTimeMillis());
if (trafficShapingHandler != null) {
trafficShapingHandler.doAccounting(this);
}
if (scheduledFuture != null) {
scheduledFuture.cancel(true);
}
public synchronized void stop() {
if (!monitorActive.get()) {
return;
}
monitorActive.set(false);
resetAccounting(System.currentTimeMillis());
if (trafficShapingHandler != null) {
trafficShapingHandler.doAccounting(this);
}
if (scheduledFuture != null) {
scheduledFuture.cancel(true);
}
}

Expand All @@ -202,20 +198,18 @@ public void stop() {
*
* @param newLastTime the millisecond unix timestamp that we should be considered up-to-date for
*/
void resetAccounting(long newLastTime) {
synchronized (lastTime) {
long interval = newLastTime - lastTime.getAndSet(newLastTime);
if (interval == 0) {
// nothing to do
return;
}
lastReadBytes = currentReadBytes.getAndSet(0);
lastWrittenBytes = currentWrittenBytes.getAndSet(0);
lastReadThroughput = lastReadBytes / interval * 1000;
// nb byte / checkInterval in ms * 1000 (1s)
lastWriteThroughput = lastWrittenBytes / interval * 1000;
// nb byte / checkInterval in ms * 1000 (1s)
synchronized void resetAccounting(long newLastTime) {
long interval = newLastTime - lastTime.getAndSet(newLastTime);
if (interval == 0) {
// nothing to do
return;
}
lastReadBytes = currentReadBytes.getAndSet(0);
lastWrittenBytes = currentWrittenBytes.getAndSet(0);
lastReadThroughput = lastReadBytes / interval * 1000;
// nb byte / checkInterval in ms * 1000 (1s)
lastWriteThroughput = lastWrittenBytes / interval * 1000;
// nb byte / checkInterval in ms * 1000 (1s)
}

/**
Expand Down

0 comments on commit e258919

Please sign in to comment.