From 0120d35a9aa7753d6642b41dc2da0b1bec97e718 Mon Sep 17 00:00:00 2001 From: Adam Date: Fri, 14 Oct 2022 16:40:33 -0400 Subject: [PATCH] Fix advancement-related memory leak (#104) Fix memory leak with loaded players' advancements Fix missing transaction transfer for player inventories Fix incorrect transaction transfer for ender chests Minor code health --- .../internal/v1_18_R2/PlayerDataManager.java | 3 ++ .../internal/v1_18_R2/SpecialEnderChest.java | 35 ++++++++++----- .../v1_18_R2/SpecialPlayerInventory.java | 45 +++++++++++++------ .../internal/v1_19_R1/PlayerDataManager.java | 3 ++ .../internal/v1_19_R1/SpecialEnderChest.java | 33 +++++++++----- .../v1_19_R1/SpecialPlayerInventory.java | 43 ++++++++++++------ .../main/java/com/lishid/openinv/OpenInv.java | 5 ++- .../openinv/internal/IPlayerDataManager.java | 2 +- .../openinv/internal/OpenInventoryView.java | 14 +++--- 9 files changed, 125 insertions(+), 58 deletions(-) diff --git a/internal/v1_18_R2/src/main/java/com/lishid/openinv/internal/v1_18_R2/PlayerDataManager.java b/internal/v1_18_R2/src/main/java/com/lishid/openinv/internal/v1_18_R2/PlayerDataManager.java index 1be5f57..127e39c 100644 --- a/internal/v1_18_R2/src/main/java/com/lishid/openinv/internal/v1_18_R2/PlayerDataManager.java +++ b/internal/v1_18_R2/src/main/java/com/lishid/openinv/internal/v1_18_R2/PlayerDataManager.java @@ -99,6 +99,9 @@ public class PlayerDataManager implements IPlayerDataManager { ServerPlayer entity = new ServerPlayer(server, worldServer, profile); + // Stop listening for advancement progression - if this is not cleaned up, loading causes a memory leak. + entity.getAdvancements().stopListening(); + try { injectPlayer(entity); } catch (IllegalAccessException e) { diff --git a/internal/v1_18_R2/src/main/java/com/lishid/openinv/internal/v1_18_R2/SpecialEnderChest.java b/internal/v1_18_R2/src/main/java/com/lishid/openinv/internal/v1_18_R2/SpecialEnderChest.java index f46ff61..e0e00a8 100644 --- a/internal/v1_18_R2/src/main/java/com/lishid/openinv/internal/v1_18_R2/SpecialEnderChest.java +++ b/internal/v1_18_R2/src/main/java/com/lishid/openinv/internal/v1_18_R2/SpecialEnderChest.java @@ -67,18 +67,29 @@ public class SpecialEnderChest extends PlayerEnderChestContainer implements ISpe @Override public void setPlayerOnline(@NotNull final org.bukkit.entity.Player player) { - if (!this.playerOnline) { - try { - this.owner = PlayerDataManager.getHandle(player); - PlayerEnderChestContainer enderChest = owner.getEnderChestInventory(); - for (int i = 0; i < enderChest.getContainerSize(); ++i) { - enderChest.setItem(i, this.items.get(i)); - } - this.items = enderChest.items; - enderChest.transaction.addAll(this.transaction); - } catch (Exception ignored) {} - this.playerOnline = true; + if (this.playerOnline) { + return; } + + ServerPlayer offlinePlayer = this.owner; + ServerPlayer onlinePlayer = PlayerDataManager.getHandle(player); + + // Set owner to new player. + this.owner = onlinePlayer; + + // Set player's ender chest contents to our modified contents. + PlayerEnderChestContainer onlineEnderChest = onlinePlayer.getEnderChestInventory(); + for (int i = 0; i < onlineEnderChest.getContainerSize(); ++i) { + onlineEnderChest.setItem(i, this.items.get(i)); + } + + // Set our item array to the new inventory's array. + this.items = onlineEnderChest.items; + + // Add viewers to new inventory. + onlineEnderChest.transaction.addAll(offlinePlayer.getEnderChestInventory().transaction); + + this.playerOnline = true; } @Override @@ -317,7 +328,7 @@ public class SpecialEnderChest extends PlayerEnderChestContainer implements ISpe @Override public String toString() { - return this.items.stream().filter((itemStack) -> !itemStack.isEmpty()).collect(Collectors.toList()).toString(); + return this.items.stream().filter((itemStack) -> !itemStack.isEmpty()).toList().toString(); } @Override diff --git a/internal/v1_18_R2/src/main/java/com/lishid/openinv/internal/v1_18_R2/SpecialPlayerInventory.java b/internal/v1_18_R2/src/main/java/com/lishid/openinv/internal/v1_18_R2/SpecialPlayerInventory.java index 04c2c5f..b007155 100644 --- a/internal/v1_18_R2/src/main/java/com/lishid/openinv/internal/v1_18_R2/SpecialPlayerInventory.java +++ b/internal/v1_18_R2/src/main/java/com/lishid/openinv/internal/v1_18_R2/SpecialPlayerInventory.java @@ -77,20 +77,37 @@ public class SpecialPlayerInventory extends Inventory implements ISpecialPlayerI @Override public void setPlayerOnline(@NotNull org.bukkit.entity.Player player) { - if (!this.playerOnline) { - Player entityPlayer = PlayerDataManager.getHandle(player); - entityPlayer.getInventory().transaction.addAll(this.transaction); - this.player = entityPlayer; - for (int i = 0; i < getContainerSize(); ++i) { - this.player.getInventory().setItem(i, getRawItem(i)); - } - this.player.getInventory().selected = this.selected; - this.items = this.player.getInventory().items; - this.armor = this.player.getInventory().armor; - this.offhand = this.player.getInventory().offhand; - this.compartments = ImmutableList.of(this.items, this.armor, this.offhand); - this.playerOnline = true; + if (this.playerOnline) { + return; } + + Player offlinePlayer = this.player; + Player onlinePlayer = PlayerDataManager.getHandle(player); + onlinePlayer.getInventory().transaction.addAll(this.transaction); + + // Set owner to new player. + this.player = onlinePlayer; + + // Set player's inventory contents to our modified contents. + Inventory onlineInventory = onlinePlayer.getInventory(); + for (int i = 0; i < getContainerSize(); ++i) { + onlineInventory.setItem(i, getRawItem(i)); + } + onlineInventory.selected = this.selected; + + // Set our item arrays to the new inventory's arrays. + this.items = onlineInventory.items; + this.armor = onlineInventory.armor; + this.offhand = onlineInventory.offhand; + this.compartments = ImmutableList.of(this.items, this.armor, this.offhand); + + // Add existing viewers to new viewer list. + Inventory offlineInventory = offlinePlayer.getInventory(); + // Remove self from listing - player is always a viewer of their own inventory, prevent duplicates. + offlineInventory.transaction.remove(offlinePlayer.getBukkitEntity()); + onlineInventory.transaction.addAll(offlineInventory.transaction); + + this.playerOnline = true; } @Override @@ -138,7 +155,7 @@ public class SpecialPlayerInventory extends Inventory implements ISpecialPlayerI } } - private static record IndexedCompartment(@Nullable NonNullList compartment, int index) {} + private record IndexedCompartment(@Nullable NonNullList compartment, int index) {} private @NotNull SpecialPlayerInventory.IndexedCompartment getIndexedContent(int index) { if (index < items.size()) { diff --git a/internal/v1_19_R1/src/main/java/com/lishid/openinv/internal/v1_19_R1/PlayerDataManager.java b/internal/v1_19_R1/src/main/java/com/lishid/openinv/internal/v1_19_R1/PlayerDataManager.java index da0232a..3534485 100644 --- a/internal/v1_19_R1/src/main/java/com/lishid/openinv/internal/v1_19_R1/PlayerDataManager.java +++ b/internal/v1_19_R1/src/main/java/com/lishid/openinv/internal/v1_19_R1/PlayerDataManager.java @@ -100,6 +100,9 @@ public class PlayerDataManager implements IPlayerDataManager { ServerPlayer entity = new ServerPlayer(server, worldServer, profile, null); + // Stop listening for advancement progression - if this is not cleaned up, loading causes a memory leak. + entity.getAdvancements().stopListening(); + try { injectPlayer(entity); } catch (IllegalAccessException e) { diff --git a/internal/v1_19_R1/src/main/java/com/lishid/openinv/internal/v1_19_R1/SpecialEnderChest.java b/internal/v1_19_R1/src/main/java/com/lishid/openinv/internal/v1_19_R1/SpecialEnderChest.java index e2a5546..d46583f 100644 --- a/internal/v1_19_R1/src/main/java/com/lishid/openinv/internal/v1_19_R1/SpecialEnderChest.java +++ b/internal/v1_19_R1/src/main/java/com/lishid/openinv/internal/v1_19_R1/SpecialEnderChest.java @@ -67,18 +67,29 @@ public class SpecialEnderChest extends PlayerEnderChestContainer implements ISpe @Override public void setPlayerOnline(@NotNull final org.bukkit.entity.Player player) { - if (!this.playerOnline) { - try { - this.owner = PlayerDataManager.getHandle(player); - PlayerEnderChestContainer enderChest = owner.getEnderChestInventory(); - for (int i = 0; i < enderChest.getContainerSize(); ++i) { - enderChest.setItem(i, this.items.get(i)); - } - this.items = enderChest.items; - enderChest.transaction.addAll(this.transaction); - } catch (Exception ignored) {} - this.playerOnline = true; + if (this.playerOnline) { + return; } + + ServerPlayer offlinePlayer = this.owner; + ServerPlayer onlinePlayer = PlayerDataManager.getHandle(player); + + // Set owner to new player. + this.owner = onlinePlayer; + + // Set player's ender chest contents to our modified contents. + PlayerEnderChestContainer onlineEnderChest = onlinePlayer.getEnderChestInventory(); + for (int i = 0; i < onlineEnderChest.getContainerSize(); ++i) { + onlineEnderChest.setItem(i, this.items.get(i)); + } + + // Set our item array to the new inventory's array. + this.items = onlineEnderChest.items; + + // Add viewers to new inventory. + onlineEnderChest.transaction.addAll(offlinePlayer.getEnderChestInventory().transaction); + + this.playerOnline = true; } @Override diff --git a/internal/v1_19_R1/src/main/java/com/lishid/openinv/internal/v1_19_R1/SpecialPlayerInventory.java b/internal/v1_19_R1/src/main/java/com/lishid/openinv/internal/v1_19_R1/SpecialPlayerInventory.java index d819e35..cc5ecbd 100644 --- a/internal/v1_19_R1/src/main/java/com/lishid/openinv/internal/v1_19_R1/SpecialPlayerInventory.java +++ b/internal/v1_19_R1/src/main/java/com/lishid/openinv/internal/v1_19_R1/SpecialPlayerInventory.java @@ -77,20 +77,37 @@ public class SpecialPlayerInventory extends Inventory implements ISpecialPlayerI @Override public void setPlayerOnline(@NotNull org.bukkit.entity.Player player) { - if (!this.playerOnline) { - Player entityPlayer = PlayerDataManager.getHandle(player); - entityPlayer.getInventory().transaction.addAll(this.transaction); - this.player = entityPlayer; - for (int i = 0; i < getContainerSize(); ++i) { - this.player.getInventory().setItem(i, getRawItem(i)); - } - this.player.getInventory().selected = this.selected; - this.items = this.player.getInventory().items; - this.armor = this.player.getInventory().armor; - this.offhand = this.player.getInventory().offhand; - this.compartments = ImmutableList.of(this.items, this.armor, this.offhand); - this.playerOnline = true; + if (this.playerOnline) { + return; } + + Player offlinePlayer = this.player; + Player onlinePlayer = PlayerDataManager.getHandle(player); + onlinePlayer.getInventory().transaction.addAll(this.transaction); + + // Set owner to new player. + this.player = onlinePlayer; + + // Set player's inventory contents to our modified contents. + Inventory onlineInventory = onlinePlayer.getInventory(); + for (int i = 0; i < getContainerSize(); ++i) { + onlineInventory.setItem(i, getRawItem(i)); + } + onlineInventory.selected = this.selected; + + // Set our item arrays to the new inventory's arrays. + this.items = onlineInventory.items; + this.armor = onlineInventory.armor; + this.offhand = onlineInventory.offhand; + this.compartments = ImmutableList.of(this.items, this.armor, this.offhand); + + // Add existing viewers to new viewer list. + Inventory offlineInventory = offlinePlayer.getInventory(); + // Remove self from listing - player is always a viewer of their own inventory, prevent duplicates. + offlineInventory.transaction.remove(offlinePlayer.getBukkitEntity()); + onlineInventory.transaction.addAll(offlineInventory.transaction); + + this.playerOnline = true; } @Override diff --git a/plugin/src/main/java/com/lishid/openinv/OpenInv.java b/plugin/src/main/java/com/lishid/openinv/OpenInv.java index 06cc68f..98b634e 100644 --- a/plugin/src/main/java/com/lishid/openinv/OpenInv.java +++ b/plugin/src/main/java/com/lishid/openinv/OpenInv.java @@ -476,8 +476,9 @@ public class OpenInv extends JavaPlugin implements IOpenInv { if (!disableSaving() && current != null - && current.getPlayer() instanceof Player player && !player.isOnline()) { - this.accessor.getPlayerDataManager().inject(player).saveData(); + && current.getPlayer() instanceof Player player + && !player.isOnline()) { + this.accessor.getPlayerDataManager().inject(player).saveData(); } }); } diff --git a/plugin/src/main/java/com/lishid/openinv/internal/IPlayerDataManager.java b/plugin/src/main/java/com/lishid/openinv/internal/IPlayerDataManager.java index 3778054..ca66932 100644 --- a/plugin/src/main/java/com/lishid/openinv/internal/IPlayerDataManager.java +++ b/plugin/src/main/java/com/lishid/openinv/internal/IPlayerDataManager.java @@ -1,5 +1,5 @@ /* - * Copyright (C) 2011-2021 lishid. All rights reserved. + * Copyright (C) 2011-2022 lishid. All rights reserved. * * This program is free software: you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by diff --git a/plugin/src/main/java/com/lishid/openinv/internal/OpenInventoryView.java b/plugin/src/main/java/com/lishid/openinv/internal/OpenInventoryView.java index 7087e3d..a7f4a95 100644 --- a/plugin/src/main/java/com/lishid/openinv/internal/OpenInventoryView.java +++ b/plugin/src/main/java/com/lishid/openinv/internal/OpenInventoryView.java @@ -27,13 +27,17 @@ import org.jetbrains.annotations.NotNull; public class OpenInventoryView extends InventoryView { - private final Player player; - private final ISpecialInventory inventory; - private final String titleKey; - private final String titleDefaultSuffix; + private final @NotNull Player player; + private final @NotNull ISpecialInventory inventory; + private final @NotNull String titleKey; + private final @NotNull String titleDefaultSuffix; private String title; - public OpenInventoryView(Player player, ISpecialInventory inventory, String titleKey, String titleDefaultSuffix) { + public OpenInventoryView( + @NotNull Player player, + @NotNull ISpecialInventory inventory, + @NotNull String titleKey, + @NotNull String titleDefaultSuffix) { this.player = player; this.inventory = inventory; this.titleKey = titleKey;