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. latest.log
          11 kB
          Michael Hurt
        2. crash-2018-07-28_02.30.14-server.txt
          22 kB
          Michael Hurt

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

              Created:
              Updated:
              Resolved: