This is not thread-safe.

[creswick]

  public long next() {
    // This needs to be inside the synchronized block:
    long currentTime = System.currentTimeMillis();

Without that change, two threads (call them thread-a and thread-b) call next();

• Thread-a gets time t
• Thread-b gets time t+1
• Thread-a is interrupted for some reason (due to cpu scheduling, for example)
• Thread-b enters the synchronized block; and sets referenceTime to to time t+1.
• Thread-a blocks on synchronized block.
• Thread-b exits the block, relinquishing the lock.
• Thread-a enters the synchronized block, and compares it's currentTime (t) with the referenceTime (t+1), and throws a RuntimeException.

[williamvoor]

I've been seeing the exception "Last referenceTime %s is after reference time %s" when running my application unit tests. Putting the entire block inside syncronized solves the problem.

[GuiSim]

Wouldn't a simple fix be to change the code to have the whole method synchronized ?

[creswick]

@GuiSim yeah, that should work. I don't think it is necessary, though.

I was concerned that the computation in the return statement is actually a significant amount of the time spent in this method (three shifts and two ORs, compared to the system call, three comparisons an increment and an assignment). Although I haven't looked at the bytecode to confirm that.

[GuiSim]

What do you mean by "not necessary" @creswick ?
As far as I know, this issue has not been resolved. Are you saying there's a more efficient way to solve the bug?

[creswick]

@GuiSim All I meant was that the last line in the function does not need to be in the critical section.

This is what I suggest (I'm not making an PR, because I do not currently have a java dev environment set up and as such I can't even test that this compiles -- let alone test it. I believe this works fine, I'm just being cautious about implying that it works without actually compiling / running it.)

public long next() {
    // these must be *declared* outside of the synchronized scope, because they are
   // used in the return statement, which is outside the synchronized region:
    long counter;
    long currentTime;


    ///////////////////////////////////////////////////////////////////////////////////////////////////////////
    //
    //   START SYNCHRONIZED SECTION
    synchronized(this) {
      // currentTime needs to be *set* in the synchronized section:
      currentTime = System.currentTimeMillis();

      if (currentTime < referenceTime) {
        throw new RuntimeException(String.format("Last referenceTime %s is after reference time %s", referenceTime, currentTime));
      } else if (currentTime > referenceTime) {
        this.sequence = 0;
      } else {
        if (this.sequence < Snowflake.MAX_SEQUENCE) {
          this.sequence++;
        } else {
          throw new RuntimeException("Sequence exhausted at " + this.sequence);
        }
      }
      // The counter is a *function-local* copy of the sequence -- this assignment must be in the 
      // synchronized block, because another thread could be modifying this.sequence if it were not:
      counter = this.sequence;
      // similarly, referenceTime is a class variable, so assigning to it needs to be in the
      // synchronized section also. (I'd probably write it as `this.referenceTime`
      // or better, `this._referenceTime`, but that's just my naming preference for private fields.)
      referenceTime = currentTime;
    } 
    // 
    //                    END OF SYNCHRONIZED SECTION
    ////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////

    // Nothing in this return statement needs to be synchronized.  `currentTime` and `counter`
    // are both function local, and as such, they are thread-safe (because there are no threads 
    // declared in side this function).
    // `node` is never modified after class instantiation so it is also safe to read without 
    // synchronization -- it should (IMO) be declared final.  NODE_SHIFT and SEQ_SHIFT are final,
    //  so they are also thread safe. 
    return currentTime << NODE_SHIFT << SEQ_SHIFT | node << SEQ_SHIFT | counter;
  }

Making the whole function synchronized would put the return statement inside the synchornization block. It is certainly safe to do that, but it will be slower, because the computation of the return value will be serialized. If the return value is not in the synchronized block, then each thread can run that computation in parallel.