Uploaded image for project: 'Minecraft: Java Edition'
  1. Minecraft: Java Edition
  2. MC-133371

1.13(+) server tick logic is flawed

XMLWordPrintable

    • Icon: Bug Bug
    • Resolution: Invalid
    • None
    • Minecraft 1.13-pre7
    • None
    • Unconfirmed

      The bug

      The server tick logic (MinecraftServer#run()) was changed in one of the 1.13 snapshots or pre-releases. The new logic appears to be faulty (the previous was faulty as well though, see MC-121196).
      In the section "Suggested fix" below is a suggested fix and I would strongly advise to use it or at least to use parts of it.

      1.13-pre7
      long l2 = k.b() - this.aa;
      if (l2 > 2000 && this.aa - this.Q >= 15000) {
          long l3 = l2 / 50;
          h.warn("Can't keep up! Is the server overloaded? Running {}ms or {} ticks behind", (Object)l2, (Object)l3);
          this.aa += l3 * 50;
          this.Q = this.aa;
      }
      this.v();
      this.aa += 50;
      while (k.b() < this.aa) {
          Thread.sleep(1);
      }
      this.P = true;
      
      1.13-pre7 deobfuscated
      long backlogMillis = currentMillis() - this.lastTickExpectedEnd;
      
      if (backlogMillis > 2000 && this.lastTickExpectedEnd - this.lastWarnTime >= 15000) {
          long backlogTicks = backlogMillis / 50;
          h.warn("Can't keep up! Is the server overloaded? Running {}ms or {} ticks behind", (Object)backlogMillis, (Object)backlogTicks);
          this.lastTickExpectedEnd += backlogTicks * 50;
          this.lastWarnTime = this.lastTickExpectedEnd;
      }
      
      this.tick();
      this.lastTickExpectedEnd += 50;
      
      while (currentMillis() < this.lastTickExpectedEnd) {
          Thread.sleep(1);
      }
      
      this.isServerRunning = true;
      

      It appears the line containing the backlog logic is incorrect, it should subtract the backlog ticks. Otherwise a server already running behind will sleep even longer, is should be:

      this.lastTickExpectedEnd -= backlogTicks * 50;
      

      Assuming that is the case, there are still the following problems:

      • Possible problems when nanoTime() (used for currentMillis()) overflows; the Java docs suggest to perform comparisons as "t1 - t0 < 0, not t1 < t0"
      • Thread.sleep(1) might switch control too often to the server thread even if it still has not reached lastTickExpectedEnd yet
      • The server is not able to catch up after running behind without accumulating the backlog to 2000 milliseconds, because only then the lastTickExpectedEnd is adjusted; and always 50 milliseconds are added to the expected end
      • It always adds 50 milliseconds to the expected end, even if the backlogTicks had an remainder (this is related to the point above, not being able to catch up)
      • The lastWarnTime depends on the tick speed instead of directly using currentMillis()

      Suggested fix

      The following suggested fix should address all issues mentioned above. Additionally it runs backlog ticks in a loop and does not query the current millis for each iteration then.
      Feel free to use it; the author does not have to be mentioned.

      At the moment this fix is only theoretical and not tested yet.

      Field changes:

      • Remove lastTickExpectedEnd
      • Add new field this.lastTickEnd, initialized with currentMillis()

      Two new local variables outside the while (this.serverRunning) loop are needed:

      • backlogMillis = 0
      • static maxTickLengthMillis = 50

      The following is the content of the while (this.serverRunning) loop:

      backlogMillis += maxTickLengthMillis;
      
      while (backlogMillis >= maxTickLengthMillis) {
          backlogMillis -= maxTickLengthMillis;
          tick();
          this.serverIsRunning = true;
      }
      
      long currentMillis = currentMillis();
      long remainingMillis = maxTickLengthMillis - (currentMillis - this.lastTickEnd) - backlogMillis;
      
      if (remainingMillis > 0) {
          Thread.sleep(remainingMillis);
          this.lastTickEnd = currentMillis + remainingMillis;
          backlogMillis = 0;
      }
      else {
          this.lastTickEnd = currentMillis;
          backlogMillis = -remainingMillis;
          // Position to show warnings; use currentMillis as time value
      }
      

      Notes

      • The field isServerRunning can probably be removed since it looks like it is not used anymore
      • The watchdog code has to be changed; whatever is used to signal that the server is still running, it has to be in the loop containing the call to tick()

        1. crash-2018-07-28_02.30.14-server.txt
          22 kB
          Michael Hurt
        2. latest.log
          11 kB
          Michael Hurt

            Unassigned Unassigned
            marcono1234 Marcono1234
            Votes:
            1 Vote for this issue
            Watchers:
            1 Start watching this issue

              Created:
              Updated:
              Resolved: