diff --git a/AdvancedCore/src/main/java/com/bencodez/advancedcore/api/item/FullInventoryHandler.java b/AdvancedCore/src/main/java/com/bencodez/advancedcore/api/item/FullInventoryHandler.java index 029cdd8423..448e0b14f7 100644 --- a/AdvancedCore/src/main/java/com/bencodez/advancedcore/api/item/FullInventoryHandler.java +++ b/AdvancedCore/src/main/java/com/bencodez/advancedcore/api/item/FullInventoryHandler.java @@ -1,20 +1,27 @@ package com.bencodez.advancedcore.api.item; import java.util.ArrayList; +import java.util.Collection; import java.util.HashMap; -import java.util.Map; import java.util.Map.Entry; import java.util.UUID; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.Executors; import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.ScheduledFuture; import java.util.concurrent.TimeUnit; +import java.util.concurrent.locks.ReentrantReadWriteLock; +import java.util.logging.Level; import org.bukkit.Bukkit; +import org.bukkit.configuration.ConfigurationSection; +import org.bukkit.configuration.file.FileConfiguration; +import org.bukkit.configuration.file.YamlConfiguration; import org.bukkit.entity.Player; import org.bukkit.inventory.ItemStack; import com.bencodez.advancedcore.AdvancedCorePlugin; +import com.bencodez.advancedcore.data.ServerData; import com.bencodez.simpleapi.messages.MessageAPI; import lombok.Getter; @@ -23,223 +30,267 @@ * Handler for items when player inventories are full. */ public class FullInventoryHandler { - /** - * The items waiting to be given. - * - * @return the items waiting to be given - */ + private static final long MESSAGE_COOLDOWN_MS = 5000L; + private static final long PENDING_ITEM_RETENTION_MS = TimeUnit.DAYS.toMillis(1); + @Getter - private ConcurrentHashMap> items; + private final ConcurrentHashMap> items = new ConcurrentHashMap<>(); - private AdvancedCorePlugin plugin; + private final AdvancedCorePlugin plugin; + private final ReentrantReadWriteLock deliveryLock = new ReentrantReadWriteLock(true); - /** - * The timer executor service. - * - * @return the timer executor service - */ @Getter private ScheduledExecutorService timer; - /** - * The last message time for each player. - * - * @return the last message time for each player - */ + private ScheduledFuture checkTask; + @Getter - private ConcurrentHashMap lastMessageTime; + private final ConcurrentHashMap lastMessageTime = new ConcurrentHashMap<>(); - /** - * Constructor for FullInventoryHandler. - * - * @param plugin the plugin instance - */ public FullInventoryHandler(AdvancedCorePlugin plugin) { - items = new ConcurrentHashMap<>(); - lastMessageTime = new ConcurrentHashMap<>(); this.plugin = plugin; loadTimer(); startup(); } - /** - * Adds multiple items for a player. - * - * @param uuid the player UUID - * @param item the items to add - */ public void add(UUID uuid, ArrayList item) { - if (items.containsKey(uuid)) { - ArrayList current = items.get(uuid); - current.addAll(item); - items.put(null, current); - } else { - items.put(uuid, item); - } + addItems(uuid, item); } - /** - * Adds a single item for a player. - * - * @param uuid the player UUID - * @param item the item to add - */ public void add(UUID uuid, ItemStack item) { - if (items.containsKey(uuid)) { - ArrayList current = items.get(uuid); - current.add(item); - items.put(uuid, current); - } else { - ArrayList itemList = new ArrayList<>(); - itemList.add(item); - items.put(uuid, itemList); + if (item == null) { + return; } + ArrayList itemList = new ArrayList<>(); + itemList.add(item); + addItems(uuid, itemList); } - /** - * Checks all players for pending items. - */ public void check() { - for (UUID entry : items.keySet()) { - Player p = Bukkit.getPlayer(entry); - check(p); - if (lastMessageTime.containsKey(entry)) { - if ((System.currentTimeMillis() - lastMessageTime.get(p.getUniqueId()).longValue()) > 5000) { - lastMessageTime.remove(entry); - } - } + if (!Bukkit.isPrimaryThread()) { + plugin.getBukkitScheduler().runTask(plugin, this::schedulePendingPlayerChecks); + return; } + schedulePendingPlayerChecks(); } - /** - * Checks a specific player for pending items. - * - * @param p the player - */ - public void check(Player p) { - if (p != null && items.containsKey(p.getUniqueId())) { - ArrayList extra = new ArrayList<>(); - for (ItemStack item : items.get(p.getUniqueId())) { - HashMap excess = p.getInventory().addItem(item); - for (Map.Entry me : excess.entrySet()) { - extra.add(me.getValue()); - } - } - if (extra.size() == 0) { - items.remove(p.getUniqueId()); - } else { - items.put(p.getUniqueId(), extra); - } + public void check(Player player) { + if (player == null) { + return; } + plugin.getBukkitScheduler().runTask(plugin, () -> checkOwnedPlayer(player), player); } - /** - * Gives items to a player. - * - * @param p the player - * @param item the items to give - */ - public void giveItem(Player p, ItemStack... item) { - HashMap excess = p.getInventory().addItem(item); - boolean full = false; - boolean dropItems = plugin.getOptions().isDropOnFullInv(); - - for (Map.Entry me : excess.entrySet()) { - full = true; - if (dropItems) { - p.getWorld().dropItem(p.getLocation(), me.getValue()); - } else { - add(p.getUniqueId(), me.getValue()); - } + public void giveItem(Player player, ItemStack... item) { + if (player == null || item == null || item.length == 0) { + return; } - if (full) { - if (lastMessageTime.containsKey(p.getUniqueId())) { - if ((System.currentTimeMillis() - lastMessageTime.get(p.getUniqueId()).longValue()) > 5000) { - sendMessage(p); - } - } else { - sendMessage(p); - } + ItemStack[] itemsToGive = item.clone(); + plugin.getBukkitScheduler().runTask(plugin, () -> giveItemOwnedPlayer(player, itemsToGive), player); + } + public synchronized void loadTimer() { + if (timer == null || timer.isShutdown() || timer.isTerminated()) { + timer = Executors.newSingleThreadScheduledExecutor(); + checkTask = null; + } + if (checkTask != null && !checkTask.isDone() && !checkTask.isCancelled()) { + return; } + checkTask = timer.scheduleAtFixedRate( + () -> plugin.getBukkitScheduler().runTask(plugin, this::schedulePendingPlayerChecks), 10, 30, + TimeUnit.SECONDS); + } - p.updateInventory(); + public synchronized void shutdown() { + if (checkTask != null) { + checkTask.cancel(false); + checkTask = null; + } + if (timer != null) { + timer.shutdownNow(); + } } /** - * Loads the timer for checking inventories. + * Saves pending items to disk. The complete replacement section is first built in + * a temporary configuration. Only after that succeeds is the live configuration + * replaced in memory, followed by a single disk save. Delivery operations hold a + * shared lock, while saving holds the exclusive lock, so a snapshot cannot observe + * the temporary remove/re-add state of an in-flight inventory check. */ - public void loadTimer() { - timer = Executors.newScheduledThreadPool(1); - timer.scheduleAtFixedRate(new Runnable() { + public void save() { + deliveryLock.writeLock().lock(); + try { + ServerData serverData = plugin.getServerDataFile(); + if (serverData == null || serverData.getData() == null) { + return; + } - @Override - public void run() { - check(); + YamlConfiguration snapshot = new YamlConfiguration(); + long now = System.currentTimeMillis(); + for (Entry> entry : items.entrySet()) { + ArrayList pending = new ArrayList<>(entry.getValue()); + String basePath = "FullInventory." + entry.getKey(); + for (int i = 0; i < pending.size(); i++) { + snapshot.set(basePath + ".Items." + i, pending.get(i)); + } + snapshot.set(basePath + ".Time", now); + } + + FileConfiguration data = serverData.getData(); + data.set("FullInventory", null); + ConfigurationSection replacement = snapshot.getConfigurationSection("FullInventory"); + if (replacement != null) { + for (String path : replacement.getKeys(true)) { + if (!replacement.isConfigurationSection(path)) { + data.set("FullInventory." + path, replacement.get(path)); + } + } } - }, 10, 30, TimeUnit.SECONDS); + serverData.saveData(); + } catch (Exception e) { + plugin.getLogger().log(Level.WARNING, "Failed to save pending full-inventory items", e); + } finally { + deliveryLock.writeLock().unlock(); + } } - /** - * Saves pending items to disk. - */ - public void save() { + public void startup() { try { - if (plugin.getServerDataFile().getData() == null) { + if (plugin.getServerDataFile() == null || plugin.getServerDataFile().getData() == null) { return; } - for (Entry> entry : items.entrySet()) { - ArrayList items = entry.getValue(); - for (int i = 0; i < items.size(); i++) { - plugin.getServerDataFile().setData("FullInventory." + entry.getKey().toString() + ".Items." + i, - items.get(i)); + ConfigurationSection root = plugin.getServerDataFile().getData().getConfigurationSection("FullInventory"); + if (root == null) { + return; + } + + long now = System.currentTimeMillis(); + for (String uuidString : root.getKeys(false)) { + try { + UUID uuid = UUID.fromString(uuidString); + long time = root.getLong(uuidString + ".Time"); + if (now - time >= PENDING_ITEM_RETENTION_MS) { + continue; + } + ConfigurationSection itemSection = root.getConfigurationSection(uuidString + ".Items"); + if (itemSection == null) { + continue; + } + for (String itemNumber : itemSection.getKeys(false)) { + ItemStack item = itemSection.getItemStack(itemNumber); + if (item != null) { + add(uuid, item); + } + } + } catch (IllegalArgumentException e) { + plugin.getLogger().warning("Skipping invalid FullInventory UUID entry: " + uuidString); } - plugin.getServerDataFile().setData("FullInventory." + entry.getKey().toString() + ".Time", - System.currentTimeMillis()); } + + plugin.getServerDataFile().setData("FullInventory", null); } catch (Exception e) { - e.printStackTrace(); + plugin.getLogger().log(Level.WARNING, "Failed to load pending full-inventory items", e); } } - private void sendMessage(Player p) { - String msg = MessageAPI.colorize(AdvancedCorePlugin.getInstance().getOptions().getFormatInvFull()); - if (!msg.isEmpty()) { - p.sendMessage(msg); - lastMessageTime.put(p.getUniqueId(), System.currentTimeMillis()); + private void addItems(UUID uuid, Collection itemsToAdd) { + if (uuid == null || itemsToAdd == null || itemsToAdd.isEmpty()) { + return; + } + deliveryLock.readLock().lock(); + try { + items.compute(uuid, (key, current) -> { + ArrayList merged = current == null ? new ArrayList<>() : new ArrayList<>(current); + for (ItemStack item : itemsToAdd) { + if (item != null) { + merged.add(item); + } + } + return merged.isEmpty() ? null : merged; + }); + } finally { + deliveryLock.readLock().unlock(); } } - /** - * Loads pending items from disk on startup. - */ - public void startup() { + private void checkOwnedPlayer(Player player) { + deliveryLock.readLock().lock(); try { - if (plugin.getServerDataFile().getData() == null - || !plugin.getServerDataFile().getData().isConfigurationSection("FullInventory")) { + UUID uuid = player.getUniqueId(); + ArrayList pending = items.remove(uuid); + if (pending == null || pending.isEmpty()) { return; } - for (String uuid : plugin.getServerDataFile().getData().getConfigurationSection("FullInventory") - .getKeys(false)) { + ArrayList extra = new ArrayList<>(); + for (ItemStack item : pending) { + if (item == null) { + continue; + } + HashMap excess = player.getInventory().addItem(item); + extra.addAll(excess.values()); + } + if (!extra.isEmpty()) { + addItems(uuid, extra); + } + } finally { + deliveryLock.readLock().unlock(); + } + } - // check time to keep a lot of items from long time offline players - long time = plugin.getServerDataFile().getData().getLong("FullInventory." + uuid + ".Time"); - if (System.currentTimeMillis() - time < (1000 * 60 * 60 * 24)) { + private void giveItemOwnedPlayer(Player player, ItemStack[] item) { + deliveryLock.readLock().lock(); + try { + HashMap excess = player.getInventory().addItem(item); + if (excess.isEmpty()) { + player.updateInventory(); + return; + } - for (String itemnum : plugin.getServerDataFile().getData() - .getConfigurationSection("FullInventory." + uuid + ".Items").getKeys(false)) { - add(UUID.fromString(uuid), plugin.getServerDataFile().getData() - .getItemStack("FullInventory." + uuid + ".Items." + itemnum)); - } + boolean dropItems = plugin.getOptions().isDropOnFullInv(); + for (ItemStack extra : excess.values()) { + if (dropItems) { + player.getWorld().dropItem(player.getLocation(), extra); + } else { + add(player.getUniqueId(), extra); } } - plugin.getServerDataFile().setData("FullInventory", null); - } catch (Exception e) { - e.printStackTrace(); + if (shouldSendMessage(player.getUniqueId())) { + sendMessage(player); + } + player.updateInventory(); + } finally { + deliveryLock.readLock().unlock(); } + } + private void schedulePendingPlayerChecks() { + long now = System.currentTimeMillis(); + for (UUID uuid : new ArrayList<>(items.keySet())) { + Player player = Bukkit.getPlayer(uuid); + if (player != null) { + plugin.getBukkitScheduler().runTask(plugin, () -> checkOwnedPlayer(player), player); + } + Long lastMessage = lastMessageTime.get(uuid); + if (lastMessage != null && now - lastMessage.longValue() > MESSAGE_COOLDOWN_MS) { + lastMessageTime.remove(uuid, lastMessage); + } + } } + private boolean shouldSendMessage(UUID uuid) { + Long lastMessage = lastMessageTime.get(uuid); + return lastMessage == null || System.currentTimeMillis() - lastMessage.longValue() > MESSAGE_COOLDOWN_MS; + } + + private void sendMessage(Player player) { + String msg = MessageAPI.colorize(plugin.getOptions().getFormatInvFull()); + if (!msg.isEmpty()) { + player.sendMessage(msg); + lastMessageTime.put(player.getUniqueId(), System.currentTimeMillis()); + } + } } diff --git a/AdvancedCore/src/test/java/com/bencodez/advancedcore/tests/item/FullInventoryHandlerTest.java b/AdvancedCore/src/test/java/com/bencodez/advancedcore/tests/item/FullInventoryHandlerTest.java new file mode 100644 index 0000000000..36a345335e --- /dev/null +++ b/AdvancedCore/src/test/java/com/bencodez/advancedcore/tests/item/FullInventoryHandlerTest.java @@ -0,0 +1,252 @@ +package com.bencodez.advancedcore.tests.item; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNotSame; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.mockStatic; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.UUID; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.TimeUnit; + +import org.bukkit.Bukkit; +import org.bukkit.configuration.ConfigurationSection; +import org.bukkit.configuration.file.YamlConfiguration; +import org.bukkit.entity.Player; +import org.bukkit.inventory.ItemStack; +import org.bukkit.inventory.PlayerInventory; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; +import org.mockito.ArgumentCaptor; +import org.mockito.MockedStatic; + +import com.bencodez.advancedcore.AdvancedCorePlugin; +import com.bencodez.advancedcore.api.item.FullInventoryHandler; +import com.bencodez.advancedcore.data.ServerData; +import com.bencodez.simpleapi.scheduler.BukkitScheduler; + +public class FullInventoryHandlerTest { + private final List handlers = new ArrayList<>(); + + @AfterEach + public void shutdownHandlers() { + for (FullInventoryHandler handler : handlers) { + handler.shutdown(); + } + } + + @Test + public void addListMergesUnderSameUuid() { + Fixture fixture = createFixture(); + UUID uuid = UUID.randomUUID(); + ItemStack first = mock(ItemStack.class); + ItemStack second = mock(ItemStack.class); + + fixture.handler.add(uuid, new ArrayList<>(java.util.List.of(first))); + fixture.handler.add(uuid, new ArrayList<>(java.util.List.of(second))); + + assertEquals(java.util.List.of(first, second), fixture.handler.getItems().get(uuid)); + assertEquals(1, fixture.handler.getItems().size()); + } + + @Test + public void offlinePlayerDoesNotBreakPendingSweep() { + Fixture fixture = createFixture(); + UUID uuid = UUID.randomUUID(); + fixture.handler.add(uuid, mock(ItemStack.class)); + fixture.handler.getLastMessageTime().put(uuid, 0L); + + try (MockedStatic bukkit = mockStatic(Bukkit.class)) { + bukkit.when(Bukkit::isPrimaryThread).thenReturn(true); + bukkit.when(() -> Bukkit.getPlayer(uuid)).thenReturn(null); + + assertDoesNotThrow(() -> fixture.handler.check()); + } + + assertFalse(fixture.handler.getLastMessageTime().containsKey(uuid)); + } + + @Test + public void pendingSweepSchedulesEachOnlinePlayerOnEntityScheduler() { + Fixture fixture = createFixture(); + UUID uuid = UUID.randomUUID(); + Player player = mock(Player.class); + fixture.handler.add(uuid, mock(ItemStack.class)); + + try (MockedStatic bukkit = mockStatic(Bukkit.class)) { + bukkit.when(Bukkit::isPrimaryThread).thenReturn(true); + bukkit.when(() -> Bukkit.getPlayer(uuid)).thenReturn(player); + + fixture.handler.check(); + } + + verify(fixture.bukkitScheduler).runTask(eq(fixture.plugin), any(Runnable.class), eq(player)); + } + + @Test + public void playerCheckAlwaysUsesEntityScheduler() { + Fixture fixture = createFixture(); + Player player = mock(Player.class); + + fixture.handler.check(player); + + verify(fixture.bukkitScheduler).runTask(eq(fixture.plugin), any(Runnable.class), eq(player)); + } + + @Test + public void giveItemAlwaysUsesEntityScheduler() { + Fixture fixture = createFixture(); + Player player = mock(Player.class); + ItemStack item = mock(ItemStack.class); + + fixture.handler.giveItem(player, item); + + verify(fixture.bukkitScheduler).runTask(eq(fixture.plugin), any(Runnable.class), eq(player)); + } + + @Test + public void handlerTimerIsIsolatedFromPluginInventoryTimer() { + Fixture fixture = createFixture(); + ScheduledExecutorService handlerTimer = fixture.handler.getTimer(); + + assertNotSame(fixture.sharedInventoryTimer, handlerTimer); + assertFalse(handlerTimer.isShutdown()); + + handlerTimer.shutdownNow(); + fixture.handler.loadTimer(); + + assertNotSame(handlerTimer, fixture.handler.getTimer()); + assertFalse(fixture.handler.getTimer().isShutdown()); + verify(fixture.sharedInventoryTimer, never()).shutdown(); + verify(fixture.sharedInventoryTimer, never()).shutdownNow(); + } + + @Test + public void shutdownStopsOnlyHandlerTimer() { + Fixture fixture = createFixture(); + ScheduledExecutorService handlerTimer = fixture.handler.getTimer(); + + fixture.handler.shutdown(); + + assertTrue(handlerTimer.isShutdown()); + verify(fixture.sharedInventoryTimer, never()).shutdown(); + verify(fixture.sharedInventoryTimer, never()).shutdownNow(); + } + + @Test + public void savePersistsCompletedSnapshotOnce() { + Fixture fixture = createFixture(); + UUID uuid = UUID.randomUUID(); + ItemStack item = mock(ItemStack.class); + fixture.data.set("FullInventory.previous.Time", 123L); + fixture.handler.add(uuid, item); + + fixture.handler.save(); + + verify(fixture.serverData).saveData(); + verify(fixture.serverData, never()).setData(eq("FullInventory"), any()); + ConfigurationSection root = fixture.data.getConfigurationSection("FullInventory"); + assertNotNull(root); + assertFalse(root.contains("previous")); + assertEquals(item, root.getItemStack(uuid + ".Items.0")); + assertTrue(root.getLong(uuid + ".Time") > 0L); + } + + @Test + public void saveWaitsForInflightDeliveryBeforeSnapshotting() throws Exception { + Fixture fixture = createFixture(); + UUID uuid = UUID.randomUUID(); + Player player = mock(Player.class); + PlayerInventory inventory = mock(PlayerInventory.class); + ItemStack pending = mock(ItemStack.class); + ItemStack excessItem = mock(ItemStack.class); + CountDownLatch deliveryStarted = new CountDownLatch(1); + CountDownLatch releaseDelivery = new CountDownLatch(1); + CountDownLatch saveFinished = new CountDownLatch(1); + + when(player.getUniqueId()).thenReturn(uuid); + when(player.getInventory()).thenReturn(inventory); + when(inventory.addItem(pending)).thenAnswer(invocation -> { + deliveryStarted.countDown(); + assertTrue(releaseDelivery.await(2, TimeUnit.SECONDS)); + HashMap excess = new HashMap<>(); + excess.put(0, excessItem); + return excess; + }); + fixture.handler.add(uuid, pending); + + fixture.handler.check(player); + ArgumentCaptor deliveryTask = ArgumentCaptor.forClass(Runnable.class); + verify(fixture.bukkitScheduler).runTask(eq(fixture.plugin), deliveryTask.capture(), eq(player)); + + Thread deliveryThread = new Thread(deliveryTask.getValue()); + deliveryThread.start(); + assertTrue(deliveryStarted.await(2, TimeUnit.SECONDS)); + + Thread saveThread = new Thread(() -> { + fixture.handler.save(); + saveFinished.countDown(); + }); + saveThread.start(); + assertFalse(saveFinished.await(100, TimeUnit.MILLISECONDS)); + + releaseDelivery.countDown(); + assertTrue(saveFinished.await(2, TimeUnit.SECONDS)); + deliveryThread.join(2000L); + saveThread.join(2000L); + + ConfigurationSection root = fixture.data.getConfigurationSection("FullInventory"); + assertNotNull(root); + assertEquals(excessItem, root.getItemStack(uuid + ".Items.0")); + } + + private Fixture createFixture() { + AdvancedCorePlugin plugin = mock(AdvancedCorePlugin.class); + ScheduledExecutorService sharedInventoryTimer = mock(ScheduledExecutorService.class); + BukkitScheduler bukkitScheduler = mock(BukkitScheduler.class); + ServerData serverData = mock(ServerData.class); + YamlConfiguration data = new YamlConfiguration(); + + when(plugin.getInventoryTimer()).thenReturn(sharedInventoryTimer); + when(plugin.getBukkitScheduler()).thenReturn(bukkitScheduler); + when(plugin.getServerDataFile()).thenReturn(serverData); + when(serverData.getData()).thenReturn(data); + + FullInventoryHandler handler = new FullInventoryHandler(plugin); + handlers.add(handler); + return new Fixture(plugin, sharedInventoryTimer, bukkitScheduler, serverData, data, handler); + } + + private static final class Fixture { + private final AdvancedCorePlugin plugin; + private final ScheduledExecutorService sharedInventoryTimer; + private final BukkitScheduler bukkitScheduler; + private final ServerData serverData; + private final YamlConfiguration data; + private final FullInventoryHandler handler; + + private Fixture(AdvancedCorePlugin plugin, ScheduledExecutorService sharedInventoryTimer, + BukkitScheduler bukkitScheduler, ServerData serverData, YamlConfiguration data, + FullInventoryHandler handler) { + this.plugin = plugin; + this.sharedInventoryTimer = sharedInventoryTimer; + this.bukkitScheduler = bukkitScheduler; + this.serverData = serverData; + this.data = data; + this.handler = handler; + } + } +}