Time monotonicity bug.

I discovered a pretty critical and wide-reaching bug in Concourse with a simple fix¹.

com.cinchapi.concourse.time.Time is used all throughout the codebase (including in critical parts like com.cinchapi.concourse.server.storage.db.Database, com.cinchapi.concourse.server.storage.Engine, com.cinchapi.concourse.server.storage.temp.Buffer, com.cinchapi.concourse.Timestamp, etc.). The problem is that com.cinchapi.concourse.time.Time.now is implemented as such:

concourse/concourse-driver-java/src/main/java/com/cinchapi/concourse/time/Time.java

Lines 38 to 40 in 8f88af2

    
           public static long now() { 
        
               return clock.time(); 
        
           }

concourse/concourse-driver-java/src/main/java/com/cinchapi/concourse/time/AtomicClock.java

Lines 37 to 48 in 8f88af2

    
           public long time() { 
        
               long now = System.currentTimeMillis() * 1000; 
        
               for (;;) { 
        
                   long lastTime = time.get(); 
        
                   if(lastTime >= now) { 
        
                       now = lastTime + 1; 
        
                   } 
        
                   if(time.compareAndSet(lastTime, now)) { 
        
                       return now; 
        
                   } 
        
               } 
        
           }

While values computed via this method are thread-safe, given the usage of System.currentTimeMillis, they don’t increase monotonically and are therefore sensitive to system clock changes, time syncs, etc. In cases of disruption, this can cause all sorts of issues, from never-ending loops (i.e., loops with conditionals dependent on these time values) to even potential data loss!

The proposed fix: Use System.nanoTime.¹

¹ The proposed fix only works when relative times [between two events] are needed, but the current usage of com.cinchapi.concourse.time.Time.now use it for more than that (e.g., usage of com.cinchapi.concourse.Timestamp); we need to address those use cases as well — I think there should be separate methods for use cases where wall-clock values are needed and those where relative times are needed. I’m happy to work on a patch for this.

CC: @jtnelson

Actually, I just debugged this again and it doesn’t have the effect that I thought it was having. This was my mistake! It actually handles the time going backward, although it doesn’t actually guarantee accuracy, so some of the usage is still incorrect. I’ll close this issue though. False alarm!

CC: @jtnelson

	public long time() {
	long now = System.currentTimeMillis() * 1000;
	for (;;) {
	long lastTime = time.get();
	if(lastTime >= now) {
	now = lastTime + 1;
	}
	if(time.compareAndSet(lastTime, now)) {
	return now;
	}
	}
	}