From 0021aeb40a2c95882dc14f16b479336d3c25a43f Mon Sep 17 00:00:00 2001 From: dordsor21 Date: Thu, 8 Oct 2020 21:15:05 +0100 Subject: [PATCH] Several fixes to actual, probable and possible synchronocity issues (#691) * Several fixes to actual, probable and possible synchronocity issues - Ensure that all edits are queued onto the same AsyncNotifyQueue by actually delegating to the parent player in PlayerProxy - Ensure that the order editsessions are being remembered on the LocalSession is being respected by using a fair ReentrentLock - Ensure a chunk cannot be called when an update is being called * Don't add locks to GetBlocks --- .../mc1_15_2/BukkitGetBlocks_1_15_2_Copy.java | 2 +- .../mc1_16_1/BukkitGetBlocks_1_16_1_Copy.java | 2 +- .../mc1_16_2/BukkitGetBlocks_1_16_2_Copy.java | 2 +- .../com/sk89q/worldedit/LocalSession.java | 196 +++++++++--------- .../platform/AbstractPlayerActor.java | 1 + .../extension/platform/PlayerProxy.java | 5 + 6 files changed, 111 insertions(+), 97 deletions(-) diff --git a/worldedit-bukkit/src/main/java/com/boydti/fawe/bukkit/adapter/mc1_15_2/BukkitGetBlocks_1_15_2_Copy.java b/worldedit-bukkit/src/main/java/com/boydti/fawe/bukkit/adapter/mc1_15_2/BukkitGetBlocks_1_15_2_Copy.java index 07b06075c..a9aef70f4 100644 --- a/worldedit-bukkit/src/main/java/com/boydti/fawe/bukkit/adapter/mc1_15_2/BukkitGetBlocks_1_15_2_Copy.java +++ b/worldedit-bukkit/src/main/java/com/boydti/fawe/bukkit/adapter/mc1_15_2/BukkitGetBlocks_1_15_2_Copy.java @@ -106,7 +106,7 @@ public class BukkitGetBlocks_1_15_2_Copy extends BukkitGetBlocks_1_15_2 { } protected void storeSection(int layer) { - update(layer, blocks[layer]); + blocks[layer] = update(layer, null).clone(); } @Override diff --git a/worldedit-bukkit/src/main/java/com/boydti/fawe/bukkit/adapter/mc1_16_1/BukkitGetBlocks_1_16_1_Copy.java b/worldedit-bukkit/src/main/java/com/boydti/fawe/bukkit/adapter/mc1_16_1/BukkitGetBlocks_1_16_1_Copy.java index 42364ed89..9b9dc3697 100644 --- a/worldedit-bukkit/src/main/java/com/boydti/fawe/bukkit/adapter/mc1_16_1/BukkitGetBlocks_1_16_1_Copy.java +++ b/worldedit-bukkit/src/main/java/com/boydti/fawe/bukkit/adapter/mc1_16_1/BukkitGetBlocks_1_16_1_Copy.java @@ -107,7 +107,7 @@ public class BukkitGetBlocks_1_16_1_Copy extends BukkitGetBlocks_1_16_1 { } protected void storeSection(int layer) { - update(layer, blocks[layer]); + blocks[layer] = update(layer, null).clone(); } @Override diff --git a/worldedit-bukkit/src/main/java/com/boydti/fawe/bukkit/adapter/mc1_16_2/BukkitGetBlocks_1_16_2_Copy.java b/worldedit-bukkit/src/main/java/com/boydti/fawe/bukkit/adapter/mc1_16_2/BukkitGetBlocks_1_16_2_Copy.java index 6981d63a7..ac80fe3f1 100644 --- a/worldedit-bukkit/src/main/java/com/boydti/fawe/bukkit/adapter/mc1_16_2/BukkitGetBlocks_1_16_2_Copy.java +++ b/worldedit-bukkit/src/main/java/com/boydti/fawe/bukkit/adapter/mc1_16_2/BukkitGetBlocks_1_16_2_Copy.java @@ -107,7 +107,7 @@ public class BukkitGetBlocks_1_16_2_Copy extends BukkitGetBlocks_1_16_2 { } protected void storeSection(int layer) { - update(layer, blocks[layer]); + blocks[layer] = update(layer, null).clone(); } @Override diff --git a/worldedit-core/src/main/java/com/sk89q/worldedit/LocalSession.java b/worldedit-core/src/main/java/com/sk89q/worldedit/LocalSession.java index 4b8345798..5094f4827 100644 --- a/worldedit-core/src/main/java/com/sk89q/worldedit/LocalSession.java +++ b/worldedit-core/src/main/java/com/sk89q/worldedit/LocalSession.java @@ -92,9 +92,12 @@ import java.util.LinkedList; import java.util.List; import java.util.ListIterator; import java.util.Map; +import java.util.Random; import java.util.TimeZone; import java.util.UUID; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReentrantLock; import java.util.stream.Collectors; import javax.annotation.Nonnull; import javax.annotation.Nullable; @@ -116,7 +119,7 @@ public class LocalSession implements TextureHolder { // Session related private transient RegionSelector selector = new CuboidRegionSelector(); private transient boolean placeAtPos1 = false; - private transient List history = Collections.synchronizedList(new LinkedList() { + private final transient List history = Collections.synchronizedList(new LinkedList() { @Override public Object get(int index) { Object value = super.get(index); @@ -135,6 +138,7 @@ public class LocalSession implements TextureHolder { private transient volatile Integer historyNegativeIndex; private transient ClipboardHolder clipboard; private transient final Object clipboardLock = new Object(); + private transient final Lock historyWriteLock = new ReentrantLock(true); private transient boolean superPickaxe = false; private transient BlockTool pickaxeMode = new SinglePickaxe(); private final transient Int2ObjectOpenHashMap tools = new Int2ObjectOpenHashMap<>(0); @@ -439,109 +443,113 @@ public class LocalSession implements TextureHolder { return null; } - public synchronized void remember(Identifiable player, World world, ChangeSet changeSet, FaweLimit limit) { - if (Settings.IMP.HISTORY.USE_DISK) { - LocalSession.MAX_HISTORY_SIZE = Integer.MAX_VALUE; - } - if (changeSet.size() == 0) { - return; - } - loadSessionHistoryFromDisk(player.getUniqueId(), world); - if (changeSet instanceof ChangeSet) { - ListIterator iter = history.listIterator(); - int i = 0; - int cutoffIndex = history.size() - getHistoryNegativeIndex(); - while (iter.hasNext()) { - Object item = iter.next(); - if (++i > cutoffIndex) { - ChangeSet oldChangeSet; - if (item instanceof ChangeSet) { - oldChangeSet = (ChangeSet) item; - } else { - oldChangeSet = getChangeSet(item); + public void remember(Identifiable player, World world, ChangeSet changeSet, FaweLimit limit) { + historyWriteLock.lock(); + try { + if (Settings.IMP.HISTORY.USE_DISK) { + LocalSession.MAX_HISTORY_SIZE = Integer.MAX_VALUE; + } + if (changeSet.size() == 0) { + return; + } + loadSessionHistoryFromDisk(player.getUniqueId(), world); + if (changeSet instanceof ChangeSet) { + ListIterator iter = history.listIterator(); + int i = 0; + int cutoffIndex = history.size() - getHistoryNegativeIndex(); + while (iter.hasNext()) { + Object item = iter.next(); + if (++i > cutoffIndex) { + ChangeSet oldChangeSet; + if (item instanceof ChangeSet) { + oldChangeSet = (ChangeSet) item; + } else { + oldChangeSet = getChangeSet(item); + } + historySize -= MainUtil.getSize(oldChangeSet); + iter.remove(); } - historySize -= MainUtil.getSize(oldChangeSet); - iter.remove(); } } + historySize += MainUtil.getSize(changeSet); + history.add(changeSet); + if (getHistoryNegativeIndex() != 0) { + setDirty(); + historyNegativeIndex = 0; + } + if (limit != null) { + int limitMb = limit.MAX_HISTORY; + while (((!Settings.IMP.HISTORY.USE_DISK && history.size() > MAX_HISTORY_SIZE) || (historySize >> 20) > limitMb) && history.size() > 1) { + ChangeSet item = (ChangeSet) history.remove(0); + item.delete(); + long size = MainUtil.getSize(item); + historySize -= size; + } + } + } finally { + historyWriteLock.unlock(); } - historySize += MainUtil.getSize(changeSet); - history.add(changeSet); - if (getHistoryNegativeIndex() != 0) { - setDirty(); - historyNegativeIndex = 0; - } - if (limit != null) { - int limitMb = limit.MAX_HISTORY; + } + + public void remember(EditSession editSession, boolean append, int limitMb) { + historyWriteLock.lock(); + try { + if (Settings.IMP.HISTORY.USE_DISK) { + LocalSession.MAX_HISTORY_SIZE = Integer.MAX_VALUE; + } + // It should have already been flushed, but just in case! + editSession.flushQueue(); + if (editSession.getChangeSet() == null || limitMb == 0 || historySize >> 20 > limitMb && !append) { + return; + } + + ChangeSet changeSet = editSession.getChangeSet(); + if (changeSet.isEmpty()) { + return; + } + + Player player = editSession.getPlayer(); + if (player != null) { + loadSessionHistoryFromDisk(player.getUniqueId(), editSession.getWorld()); + } + // Destroy any sessions after this undo point + if (append) { + ListIterator iter = history.listIterator(); + int i = 0; + int cutoffIndex = history.size() - getHistoryNegativeIndex(); + while (iter.hasNext()) { + Object item = iter.next(); + if (++i > cutoffIndex) { + ChangeSet oldChangeSet; + if (item instanceof ChangeSet) { + oldChangeSet = (ChangeSet) item; + } else { + oldChangeSet = getChangeSet(item); + } + historySize -= MainUtil.getSize(oldChangeSet); + iter.remove(); + } + } + } + + historySize += MainUtil.getSize(changeSet); + if (append) { + history.add(changeSet); + if (getHistoryNegativeIndex() != 0) { + setDirty(); + historyNegativeIndex = 0; + } + } else { + history.add(0, changeSet); + } while (((!Settings.IMP.HISTORY.USE_DISK && history.size() > MAX_HISTORY_SIZE) || (historySize >> 20) > limitMb) && history.size() > 1) { ChangeSet item = (ChangeSet) history.remove(0); item.delete(); long size = MainUtil.getSize(item); historySize -= size; } - } - } - - public synchronized void remember(EditSession editSession, boolean append, int limitMb) { - if (Settings.IMP.HISTORY.USE_DISK) { - LocalSession.MAX_HISTORY_SIZE = Integer.MAX_VALUE; - } - // It should have already been flushed, but just in case! - editSession.flushQueue(); - if (editSession.getChangeSet() == null || limitMb == 0 || historySize >> 20 > limitMb && !append) { - return; - } - /* - // Don't store anything if no changes were made - if (editSession.size() == 0) { - return; - } - */ - - ChangeSet changeSet = editSession.getChangeSet(); - if (changeSet.isEmpty()) { - return; - } - - Player player = editSession.getPlayer(); - if (player != null) { - loadSessionHistoryFromDisk(player.getUniqueId(), editSession.getWorld()); - } - // Destroy any sessions after this undo point - if (append) { - ListIterator iter = history.listIterator(); - int i = 0; - int cutoffIndex = history.size() - getHistoryNegativeIndex(); - while (iter.hasNext()) { - Object item = iter.next(); - if (++i > cutoffIndex) { - ChangeSet oldChangeSet; - if (item instanceof ChangeSet) { - oldChangeSet = (ChangeSet) item; - } else { - oldChangeSet = getChangeSet(item); - } - historySize -= MainUtil.getSize(oldChangeSet); - iter.remove(); - } - } - } - - historySize += MainUtil.getSize(changeSet); - if (append) { - history.add(changeSet); - if (getHistoryNegativeIndex() != 0) { - setDirty(); - historyNegativeIndex = 0; - } - } else { - history.add(0, changeSet); - } - while (((!Settings.IMP.HISTORY.USE_DISK && history.size() > MAX_HISTORY_SIZE) || (historySize >> 20) > limitMb) && history.size() > 1) { - ChangeSet item = (ChangeSet) history.remove(0); - item.delete(); - long size = MainUtil.getSize(item); - historySize -= size; + } finally { + historyWriteLock.unlock(); } } diff --git a/worldedit-core/src/main/java/com/sk89q/worldedit/extension/platform/AbstractPlayerActor.java b/worldedit-core/src/main/java/com/sk89q/worldedit/extension/platform/AbstractPlayerActor.java index b61589c33..ce8be7bbd 100644 --- a/worldedit-core/src/main/java/com/sk89q/worldedit/extension/platform/AbstractPlayerActor.java +++ b/worldedit-core/src/main/java/com/sk89q/worldedit/extension/platform/AbstractPlayerActor.java @@ -684,6 +684,7 @@ public abstract class AbstractPlayerActor implements Actor, Player, Cloneable { * @param async TODO description * @return false if the task was ran or queued */ + @Override public boolean runAction(Runnable ifFree, boolean checkFree, boolean async) { if (checkFree) { if (runningCount.get() != 0) { diff --git a/worldedit-core/src/main/java/com/sk89q/worldedit/extension/platform/PlayerProxy.java b/worldedit-core/src/main/java/com/sk89q/worldedit/extension/platform/PlayerProxy.java index c3a2dad04..3a8732255 100644 --- a/worldedit-core/src/main/java/com/sk89q/worldedit/extension/platform/PlayerProxy.java +++ b/worldedit-core/src/main/java/com/sk89q/worldedit/extension/platform/PlayerProxy.java @@ -83,6 +83,11 @@ public class PlayerProxy extends AbstractPlayerActor { return basePlayer.getBlockInHand(handSide); } + @Override + public boolean runAction(Runnable ifFree, boolean checkFree, boolean async) { + return basePlayer.runAction(ifFree, checkFree, async); + } + @Override public UUID getUniqueId() { return basePlayer.getUniqueId();