From 7f61a2526eab7818682ea0761e54f35f80cd2d6a Mon Sep 17 00:00:00 2001
From: Spottedleaf <Spottedleaf@users.noreply.github.com>
Date: Mon, 2 Mar 2020 17:19:06 -0800
Subject: [PATCH] #640: Fix chunk load/unload callbacks for chunk load
 cancellations

When a chunk goes from a ticket level where it is loading a
full chunk to an inactive state (i.e ticket level 33 to
ticket level 45) the full status future will be completed
with a "Right" Either (indicating unloaded). However, this
will also schedule the unload callback immediately.

However, the callback is not immediately executed. This means
the next unload/load callback that needs to be scheduled will
fail. The fix applied is to not schedule a callback if the
chunk is not loaded - if the Either is "right."

Even better, due to how completablefuture works, exceptions
are not printed by default. So the exception thrown by the
callback executor was not printed and the failure
hidden from console. This explains why no-one has tracked this issue.
Now the exception is printed so future failures with the
callback system (if any) can be tracked easier.
---
 nms-patches/PlayerChunk.patch | 50 ++++++++++++++++++++++-------------
 1 file changed, 31 insertions(+), 19 deletions(-)

diff --git a/nms-patches/PlayerChunk.patch b/nms-patches/PlayerChunk.patch
index e6be890ffd..fd52f02827 100644
--- a/nms-patches/PlayerChunk.patch
+++ b/nms-patches/PlayerChunk.patch
@@ -46,23 +46,29 @@
  
              if (either == null || either.left().isPresent()) {
                  return completablefuture;
-@@ -278,6 +287,24 @@
+@@ -278,6 +287,30 @@
          boolean flag1 = this.ticketLevel <= PlayerChunkMap.GOLDEN_TICKET;
          PlayerChunk.State playerchunk_state = getChunkState(this.oldTicketLevel);
          PlayerChunk.State playerchunk_state1 = getChunkState(this.ticketLevel);
 +        // CraftBukkit start
 +        // ChunkUnloadEvent: Called before the chunk is unloaded: isChunkLoaded is still true and chunk can still be modified by plugins.
 +        if (playerchunk_state.isAtLeast(PlayerChunk.State.BORDER) && !playerchunk_state1.isAtLeast(PlayerChunk.State.BORDER)) {
-+            this.getStatusFutureUnchecked(ChunkStatus.FULL).thenAcceptAsync((either) -> {
-+                either.ifLeft((chunkAccess) -> {
-+                    Chunk chunk = (Chunk) chunkAccess;
-+                    // Minecraft will apply the chunks tick lists to the world once the chunk got loaded, and then store the tick
-+                    // lists again inside the chunk once the chunk becomes inaccessible and set the chunk's needsSaving flag.
-+                    // These actions may however happen deferred, so we manually set the needsSaving flag already here.
-+                    chunk.setNeedsSaving(true);
-+                    chunk.unloadCallback();
-+                });
-+            }, playerchunkmap.callbackExecutor);
++            this.getStatusFutureUnchecked(ChunkStatus.FULL).thenAccept((either) -> {
++                Chunk chunk = (Chunk)either.left().orElse(null);
++                if (chunk != null) {
++                    playerchunkmap.callbackExecutor.execute(() -> {
++                        // Minecraft will apply the chunks tick lists to the world once the chunk got loaded, and then store the tick
++                        // lists again inside the chunk once the chunk becomes inaccessible and set the chunk's needsSaving flag.
++                        // These actions may however happen deferred, so we manually set the needsSaving flag already here.
++                        chunk.setNeedsSaving(true);
++                        chunk.unloadCallback();
++                    });
++                }
++            }).exceptionally((throwable) -> {
++                // ensure exceptions are printed, by default this is not the case
++                MinecraftServer.LOGGER.fatal("Failed to schedule unload callback for chunk " + PlayerChunk.this.location, throwable);
++                return null;
++            });
 +
 +            // Run callback right away if the future was already done
 +            playerchunkmap.callbackExecutor.run();
@@ -71,7 +77,7 @@
          CompletableFuture completablefuture;
  
          if (flag) {
-@@ -309,7 +336,7 @@
+@@ -309,7 +342,7 @@
          if (flag2 && !flag3) {
              completablefuture = this.fullChunkFuture;
              this.fullChunkFuture = PlayerChunk.UNLOADED_CHUNK_FUTURE;
@@ -80,19 +86,25 @@
                  playerchunkmap.getClass();
                  return either1.ifLeft(playerchunkmap::a);
              }));
-@@ -347,6 +374,20 @@
+@@ -347,6 +380,26 @@
  
          this.w.a(this.location, this::k, this.ticketLevel, this::d);
          this.oldTicketLevel = this.ticketLevel;
 +        // CraftBukkit start
 +        // ChunkLoadEvent: Called after the chunk is loaded: isChunkLoaded returns true and chunk is ready to be modified by plugins.
 +        if (!playerchunk_state.isAtLeast(PlayerChunk.State.BORDER) && playerchunk_state1.isAtLeast(PlayerChunk.State.BORDER)) {
-+            this.getStatusFutureUnchecked(ChunkStatus.FULL).thenAcceptAsync((either) -> {
-+                either.ifLeft((chunkAccess) -> {
-+                    Chunk chunk = (Chunk) chunkAccess;
-+                    chunk.loadCallback();
-+                });
-+            }, playerchunkmap.callbackExecutor);
++            this.getStatusFutureUnchecked(ChunkStatus.FULL).thenAccept((either) -> {
++                Chunk chunk = (Chunk)either.left().orElse(null);
++                if (chunk != null) {
++                    playerchunkmap.callbackExecutor.execute(() -> {
++                        chunk.loadCallback();
++                    });
++                }
++            }).exceptionally((throwable) -> {
++                // ensure exceptions are printed, by default this is not the case
++                MinecraftServer.LOGGER.fatal("Failed to schedule load callback for chunk " + PlayerChunk.this.location, throwable);
++                return null;
++            });
 +
 +            // Run callback right away if the future was already done
 +            playerchunkmap.callbackExecutor.run();