A thread-safe stop watch utility class

Question

I am writing a stop watch utility class in Java that I want to be thread-safe (concurrent). Here is what I have right now:

public class Stopwatch {

    private volatile long beginTime;
    private final AtomicLong lastLap = new AtomicLong();
    private final AtomicReference<List<Long>> laps = new AtomicReference<List<Long>>();

    public synchronized void start() {
        reset();
        beginTime = System.nanoTime();
        lastLap.set(beginTime);
    }

    public long timeElapsed() {
        return toMilliseconds(timeElapsed(beginTime));
    }

    public void lap() {
        long lap = timeElapsed(lastLap.longValue());
        lastLap.addAndGet(lap);
        getLaps().add(toMilliseconds(lap));
    }

    public synchronized void reset() {
        beginTime = 0;
        lastLap.set(0);
        if (laps.get() != null) {
            laps.get().clear();
        }
    }

    public List<Long> getLaps() {
        laps.compareAndSet(null, Collections.synchronizedList(new ArrayList<Long>()));
        return laps.get();
    }

    private long toMilliseconds(long nano) {
        return nano / 1000000;
    }

    private long timeElapsed(long start) {
        return System.nanoTime() - start;
    }
}

A few notes, the compareAndSet call in getLaps is used to lazily initialize the list. I added the synchronized modifiers to the start and reset methods to prevent the scenario of multiple threads calling those methods simultaneously, which could possibly result in some fields being reset and some being initialized, giving odd behavior. I did not add the synchronized modifiers to the other methods because I anticipate them to operate as expected even when invoked from multiple threads, though, quite frankly, I'm not entirely sure if that is the case.

I would like this implementation to avoid locking as much as possible because the locking and waiting could give slightly inaccurate results for certain methods, like timeElapsed.

Do I need to make any modifications to make this class thread-safe?

Answer 1

This class is... problematic. It is not thread safe at all. It has a mix of 3 different concurrency tools, and each of them has problems.

Volatile

volatile is almost always a bug. If you think you should be using volatile, you are wrong (almost always). Textbooks like to cover volatile, but "in the wild" it is about as common as the Sasquatch (clever pun there...).

In your code, for example, beginTime is only ever accessed from inside synchronized methods, so why is it volatile?

Atomics

You use AtomicLong to keep the lap time, but you have a race condition:

public void lap() {
    long lap = timeElapsed(lastLap.longValue());
    lastLap.addAndGet(lap);
    getLaps().add(toMilliseconds(lap));
}

In here, two threads may call lap() at about the same time. The first thread may get lastLap.LongValue() and convert it to lap. It may then pause there. The next thread may get the lap too, and call addAndGet(lap). The first thread may then overwrite the previous thread's value with the older lap. Then, both threads may get the laps List, and try to simultaneously add their lap values to the same synchronized ArrayList... the order they are added is unpredictable, but, for sure, you may end up with two really long laps, instead of a long one and a short one.

Atomics are atomic... they do not prevent race conditions (they just give a way to identify and handle them).

Synchronization

Here you have done a half-OK job. You are synchronizing on two methods, but you should be synchronizing on more.

I would recommend you throw away all the other concurrency tricks, and use only synchronization.

Believe it or not (you will believe it if you test it), synchronization blocks can be really really fast, and as a result, concurrency is still PDQ (Pretty Damn Quick).

So, with the following code, let me point out some things:

import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;

public class StopWatch {

    private long beginTime = 0;
    private long[] laps = new long[1 << 4];
    private int lapCount = 0;

    private final Object sync = new Object();

    public void start() {
        synchronized(sync) {
            if (beginTime != 0) {
                // already started
                return;
            }
            beginTime = System.nanoTime();
            laps[lapCount++] = beginTime;
        }
    }

    public long timeElapsed() {
        synchronized(sync) {
            if (beginTime == 0) {
                return 0;
            }
            return toMilliseconds(System.nanoTime() - beginTime); 
        }
    }

    public void lap() {
        synchronized(sync) {
            if (beginTime == 0) {
                return;
            }
            if (lapCount == laps.length) {
                laps = Arrays.copyOf(laps, lapCount + (lapcount >> 1) + 8);
            }
            laps[lapCount++] = System.nanoTime();
        }

    }

    public void reset() {
        synchronized(sync) {
            beginTime = 0;
            lapCount = 0;
        }
    }

    public List<Long> getLaps() {
        long[] times;
        synchronized(sync) {
            times = Arrays.copyOf(laps, lapCount);
        }
        List<Long> data = new ArrayList<>(times.length);
        for (int i = 1; i < times.length; i++) {
            data.add(toMilliseconds(times[i] - times[i - 1]));
        }
        return data;
    }

    private long toMilliseconds(long nano) {
        return nano / 1000000;
    }

}

1. I use a dedicated object sync to lock on to. This prevents someone from using my instance as a lock for their synchronization needs, and breaking my class. E.g., with your code, if someone did:

   Stopwatch sw = new Stopwatch();
   
   .....
   
   synchronized(sw) {
       Thread.sleep(10000);
   }
   

   That code would make your methods block in other threads...

2. I have no volatiles or atomics... only synchronized.

3. I use primitives, and arrays to store the data. This is fast, and reduces memory usage.
4. I don't do any math in the time-sensitive methods. All I do is store away the System.nanoTime(). The math is all done in the get methods, and outside of the synchronized blocks.

Conclusion

• Mixing concurrency strategies is bad.
• synchronization is normally more than enough for most needs
• atomics are atomic, not thread safe. Atomics provide mechanisms to identify race conditions, not avoid them.