From fbecbc167a489962c070959db68bb3d6683ce652 Mon Sep 17 00:00:00 2001 From: nossr50 Date: Fri, 1 Jan 2021 14:04:36 -0800 Subject: [PATCH] Optimizations and potentially fixing a ConcurrentModificationException involving the TransientEntityTracker Fixes #4368 --- Changelog.txt | 8 +- .../datatypes/player/PlayerProfile.java | 5 +- .../nossr50/listeners/PlayerListener.java | 11 +- src/main/java/com/gmail/nossr50/mcMMO.java | 25 ++- .../nossr50/skills/taming/TamingManager.java | 2 +- .../nossr50/util/TransientEntityTracker.java | 142 +++++++++++++----- 6 files changed, 144 insertions(+), 49 deletions(-) diff --git a/Changelog.txt b/Changelog.txt index 0a1a046a1..a829e376d 100644 --- a/Changelog.txt +++ b/Changelog.txt @@ -1,8 +1,14 @@ Version 2.1.166 + Fixed a small memory leak in the new COTW tracker + Potentially fixed a ConcurrentModificationException involving the TransientEntityTracker (report this error if you encounter it) Music discs removed from the default fishing_treasures.yml + Optimized how mcMMO saves player data (should improve timings on servers with bad disk speeds and or bad connectivity to their SQL server instance) NOTES: - No one likes fishing up music discs + No one likes fishing up music discs, if you want this change it is recommended you delete fishing_treasures.yml and let it regenerate + If any of you encounter a ConcurrentModificationException error that mentions TransientEntityTracker in its stack trace after this update let me know, I have another fix in mind for this if this update didn't fix it. + (You won't have this file if you haven't updated in a while, if so you don't need to do anything) + Version 2.1.165 Fixed a bug where Enchanted Books dropped by mcMMO (in Fishing) did not function correctly The mcMMO system which tracks player placed blocks has had some major rewrites (thanks t00thpick1) diff --git a/src/main/java/com/gmail/nossr50/datatypes/player/PlayerProfile.java b/src/main/java/com/gmail/nossr50/datatypes/player/PlayerProfile.java index 49fc3d6f7..1d1cb53c2 100644 --- a/src/main/java/com/gmail/nossr50/datatypes/player/PlayerProfile.java +++ b/src/main/java/com/gmail/nossr50/datatypes/player/PlayerProfile.java @@ -131,10 +131,11 @@ public class PlayerProfile { { saveAttempts++; - if(useSync) + //Back out of async saving if we detect a server shutdown, this is not always going to be caught + if(mcMMO.isServerShutdownExecuted() || useSync) scheduleSyncSave(); //Execute sync saves immediately else - scheduleAsyncSaveDelay(); + scheduleAsyncSave(); } else { mcMMO.p.getLogger().severe("mcMMO has failed to save the profile for " diff --git a/src/main/java/com/gmail/nossr50/listeners/PlayerListener.java b/src/main/java/com/gmail/nossr50/listeners/PlayerListener.java index bf47ef5a0..b410a330d 100644 --- a/src/main/java/com/gmail/nossr50/listeners/PlayerListener.java +++ b/src/main/java/com/gmail/nossr50/listeners/PlayerListener.java @@ -526,16 +526,15 @@ public class PlayerListener implements Listener { return; } + McMMOPlayer mcMMOPlayer = UserManager.getPlayer(player); + //Profile not loaded - if(UserManager.getPlayer(player) == null) - { + if(mcMMOPlayer == null) { return; } - McMMOPlayer mcMMOPlayer = UserManager.getPlayer(player); - //There's an issue with using Async saves on player quit - //Basically there are conditions in which an async task does not execute fast enough to save the data if the server shutdown shortly after this task was scheduled - mcMMOPlayer.logout(true); + //Use a sync save if the server is shutting down to avoid race conditions + mcMMOPlayer.logout(mcMMO.isServerShutdownExecuted()); } /** diff --git a/src/main/java/com/gmail/nossr50/mcMMO.java b/src/main/java/com/gmail/nossr50/mcMMO.java index 06347a952..f3f6bb9ac 100644 --- a/src/main/java/com/gmail/nossr50/mcMMO.java +++ b/src/main/java/com/gmail/nossr50/mcMMO.java @@ -88,6 +88,7 @@ public class mcMMO extends JavaPlugin { private static ChatManager chatManager; private static CommandManager commandManager; //ACF private static TransientEntityTracker transientEntityTracker; + private static boolean serverShutdownExecuted = false; /* Adventure */ private static BukkitAudiences audiences; @@ -292,6 +293,7 @@ public class mcMMO extends JavaPlugin { commandManager = new CommandManager(this); transientEntityTracker = new TransientEntityTracker(); + setServerShutdown(false); //Reset flag, used to make decisions about async saves } public static PlayerLevelUtils getPlayerLevelUtils() { @@ -327,6 +329,10 @@ public class mcMMO extends JavaPlugin { */ @Override public void onDisable() { + setServerShutdown(true); + //TODO: Write code to catch unfinished async save tasks, for now we just hope they finish in time, which they should in most cases + mcMMO.p.getLogger().info("Server shutdown has been executed, saving and cleaning up data..."); + try { UserManager.saveAll(); // Make sure to save player information if the server shuts down UserManager.clearAll(); @@ -345,11 +351,6 @@ public class mcMMO extends JavaPlugin { catch (Exception e) { e.printStackTrace(); } - debug("Canceling all tasks..."); - getServer().getScheduler().cancelTasks(this); // This removes our tasks - debug("Unregister all events..."); - HandlerList.unregisterAll(this); // Cancel event registrations - if (Config.getInstance().getBackupsEnabled()) { // Remove other tasks BEFORE starting the Backup, or we just cancel it straight away. try { @@ -369,6 +370,11 @@ public class mcMMO extends JavaPlugin { } } + debug("Canceling all tasks..."); + getServer().getScheduler().cancelTasks(this); // This removes our tasks + debug("Unregister all events..."); + HandlerList.unregisterAll(this); // Cancel event registrations + databaseManager.onDisable(); debug("Was disabled."); // How informative! } @@ -727,4 +733,13 @@ public class mcMMO extends JavaPlugin { public static TransientEntityTracker getTransientEntityTracker() { return transientEntityTracker; } + + public static synchronized boolean isServerShutdownExecuted() { + return serverShutdownExecuted; + } + + private static synchronized void setServerShutdown(boolean bool) { + serverShutdownExecuted = bool; + } + } diff --git a/src/main/java/com/gmail/nossr50/skills/taming/TamingManager.java b/src/main/java/com/gmail/nossr50/skills/taming/TamingManager.java index 0ec720ec1..6a902dd43 100644 --- a/src/main/java/com/gmail/nossr50/skills/taming/TamingManager.java +++ b/src/main/java/com/gmail/nossr50/skills/taming/TamingManager.java @@ -52,7 +52,7 @@ public class TamingManager extends SkillManager { lastSummonTimeStamp = 0L; //Init per-player tracking of summoned entities - mcMMO.getTransientEntityTracker().initPlayer(mmoPlayer.getPlayer().getUniqueId()); + mcMMO.getTransientEntityTracker().initPlayer(mmoPlayer.getPlayer()); //Hacky stuff used as a band-aid initStaticCaches(); diff --git a/src/main/java/com/gmail/nossr50/util/TransientEntityTracker.java b/src/main/java/com/gmail/nossr50/util/TransientEntityTracker.java index 4b22abaea..9ee1807b4 100644 --- a/src/main/java/com/gmail/nossr50/util/TransientEntityTracker.java +++ b/src/main/java/com/gmail/nossr50/util/TransientEntityTracker.java @@ -19,6 +19,7 @@ import org.jetbrains.annotations.Nullable; import java.util.*; public class TransientEntityTracker { + //These two are updated in step with each other private final @NotNull HashMap>> perPlayerTransientEntityMap; private final @NotNull HashSet chunkLookupCache; @@ -27,30 +28,74 @@ public class TransientEntityTracker { chunkLookupCache = new HashSet<>(); } - public void initPlayer(@NotNull UUID playerUUID) { - if (!isPlayerRegistered(playerUUID)) { - registerPlayer(playerUUID); + public synchronized @NotNull HashSet getChunkLookupCache() { + return chunkLookupCache; + } + + public synchronized @NotNull HashMap>> getPerPlayerTransientEntityMap() { + return perPlayerTransientEntityMap; + } + + public synchronized void initPlayer(@NotNull Player player) { + if (!isPlayerRegistered(player.getUniqueId())) { + registerPlayer(player.getUniqueId()); } } - public void cleanupPlayer(@NotNull UUID playerUUID) { - cleanupAllSummons(null, playerUUID); + /** + * Removes a player from the tracker + * + * @param playerUUID target player + */ + public synchronized void cleanupPlayer(@NotNull UUID playerUUID) { + cleanPlayer(null, playerUUID); } - public void cleanupPlayer(@NotNull Player player) { - //First remove all entities related to this player + /** + * Removes a player from the tracker + * + * @param player target player + */ + public synchronized void cleanupPlayer(@NotNull Player player) { + cleanPlayer(player, player.getUniqueId()); + } + + /** + * Removes a player from the tracker + * + * @param player target player + * @param playerUUID target player UUID + */ + private void cleanPlayer(@Nullable Player player, @NotNull UUID playerUUID) { cleanupAllSummons(player, player.getUniqueId()); + removePlayerFromMap(playerUUID); } - private boolean isPlayerRegistered(@NotNull UUID playerUUID) { - return perPlayerTransientEntityMap.get(playerUUID) != null; + private void removePlayerFromMap(@NotNull UUID playerUUID) { + getPerPlayerTransientEntityMap().remove(playerUUID); } - private void registerPlayer(@NotNull UUID playerUUID) { - perPlayerTransientEntityMap.put(playerUUID, new HashMap>()); + /** + * Checks if a player has already been registered + * Being registered constitutes having necessary values initialized in our per-player map + * + * @param playerUUID target player + * @return true if the player is registered + */ + private synchronized boolean isPlayerRegistered(@NotNull UUID playerUUID) { + return getPerPlayerTransientEntityMap().get(playerUUID) != null; + } + + /** + * Register a player to our tracker, which initializes the necessary values in our per-player map + * + * @param playerUUID player to register + */ + private synchronized void registerPlayer(@NotNull UUID playerUUID) { + getPerPlayerTransientEntityMap().put(playerUUID, new HashMap>()); for(CallOfTheWildType callOfTheWildType : CallOfTheWildType.values()) { - perPlayerTransientEntityMap.get(playerUUID).put(callOfTheWildType, new HashSet<>()); + getPerPlayerTransientEntityMap().get(playerUUID).put(callOfTheWildType, new HashSet<>()); } } @@ -60,16 +105,18 @@ public class TransientEntityTracker { * @param playerUUID the target uuid of the player * @return the tracked entities map for the player, null if the player isn't registered */ - public @Nullable HashMap> getPlayerTrackedEntityMap(@NotNull UUID playerUUID) { - return perPlayerTransientEntityMap.get(playerUUID); + public synchronized @Nullable HashMap> getPlayerTrackedEntityMap(@NotNull UUID playerUUID) { + return getPerPlayerTransientEntityMap().get(playerUUID); } - public void registerEntity(@NotNull UUID playerUUID, @NotNull TrackedTamingEntity trackedTamingEntity) { - if(!isPlayerRegistered(playerUUID)) { - mcMMO.p.getLogger().severe("Attempting to register entity to a player which hasn't been initialized!"); - initPlayer(playerUUID); - } - + /** + * Registers an entity to a player + * This includes registration to our per-player map and our chunk lookup cache + * + * @param playerUUID target player's UUID + * @param trackedTamingEntity target entity + */ + public synchronized void registerEntity(@NotNull UUID playerUUID, @NotNull TrackedTamingEntity trackedTamingEntity) { //Add to map entry getTrackedEntities(playerUUID, trackedTamingEntity.getCallOfTheWildType()).add(trackedTamingEntity); @@ -85,7 +132,7 @@ public class TransientEntityTracker { * @param callOfTheWildType target type * @return the set of tracked entities for the player, null if the player isn't registered, the set can be empty */ - private @Nullable HashSet getTrackedEntities(@NotNull UUID playerUUID, @NotNull CallOfTheWildType callOfTheWildType) { + private synchronized @Nullable HashSet getTrackedEntities(@NotNull UUID playerUUID, @NotNull CallOfTheWildType callOfTheWildType) { HashMap> playerEntityMap = getPlayerTrackedEntityMap(playerUUID); if(playerEntityMap == null) @@ -94,21 +141,45 @@ public class TransientEntityTracker { return playerEntityMap.get(callOfTheWildType); } - private void addToChunkLookupCache(@NotNull TrackedTamingEntity trackedTamingEntity) { - chunkLookupCache.add(trackedTamingEntity.getLivingEntity()); + /** + * Adds an entity to our chunk lookup cache + * + * @param trackedTamingEntity target tracked taming entity + */ + private synchronized void addToChunkLookupCache(@NotNull TrackedTamingEntity trackedTamingEntity) { + getChunkLookupCache().add(trackedTamingEntity.getLivingEntity()); } - public void unregisterEntity(@NotNull LivingEntity livingEntity) { + /** + * Removes an entity from our tracker + * This includes removal from our per-player map and our chunk lookup cache + * + * @param livingEntity target entity + */ + private void unregisterEntity(@NotNull LivingEntity livingEntity) { chunkLookupCacheCleanup(livingEntity); perPlayerTransientMapCleanup(livingEntity); } + /** + * Removes an entity from our chunk lookup cache + * + * @param livingEntity target entity + */ private void chunkLookupCacheCleanup(@NotNull LivingEntity livingEntity) { - chunkLookupCache.remove(livingEntity); + getChunkLookupCache().remove(livingEntity); } + /** + * Clean a living entity from our tracker + * Iterates over all players and their registered entities + * Doesn't do any kind of failure checking, if it doesn't find any player with a registered entity nothing bad happens or is reported + * However it should never happen like that, so maybe we could consider adding some failure to execute checking in the future + * + * @param livingEntity + */ private void perPlayerTransientMapCleanup(@NotNull LivingEntity livingEntity) { - for(UUID uuid : perPlayerTransientEntityMap.keySet()) { + for(UUID uuid : getPerPlayerTransientEntityMap().keySet()) { for(CallOfTheWildType callOfTheWildType : CallOfTheWildType.values()) { HashSet trackedEntities = getTrackedEntities(uuid, callOfTheWildType); @@ -127,10 +198,16 @@ public class TransientEntityTracker { } } - public @NotNull List getAllTransientEntitiesInChunk(@NotNull Chunk chunk) { + /** + * Get all transient entities that exist in a specific chunk + * + * @param chunk the chunk to match + * @return a list of transient entities that are located in the provided chunk + */ + public synchronized @NotNull List getAllTransientEntitiesInChunk(@NotNull Chunk chunk) { ArrayList matchingEntities = new ArrayList<>(); - for(LivingEntity livingEntity : chunkLookupCache) { + for(LivingEntity livingEntity : getChunkLookupCache()) { if(livingEntity.getLocation().getChunk().equals(chunk)) { matchingEntities.add(livingEntity); } @@ -139,17 +216,14 @@ public class TransientEntityTracker { return matchingEntities; } - /* - * Gross code below - */ - /** * Get the amount of a summon currently active for a player + * * @param playerUUID target player * @param callOfTheWildType summon type * @return the amount of summons currently active for player of target type */ - public int getAmountCurrentlySummoned(@NotNull UUID playerUUID, @NotNull CallOfTheWildType callOfTheWildType) { + public synchronized int getAmountCurrentlySummoned(@NotNull UUID playerUUID, @NotNull CallOfTheWildType callOfTheWildType) { HashSet trackedEntities = getTrackedEntities(playerUUID, callOfTheWildType); if(trackedEntities == null) @@ -165,7 +239,7 @@ public class TransientEntityTracker { * @param livingEntity entity to remove * @param player associated player */ - public void removeSummon(@NotNull LivingEntity livingEntity, @Nullable Player player, boolean timeExpired) { + public synchronized void removeSummon(@NotNull LivingEntity livingEntity, @Nullable Player player, boolean timeExpired) { //Kill the summon & remove it if(livingEntity.isValid()) { livingEntity.setHealth(0); //Should trigger entity death events