From 246d7ee0b48d35059af2f6c999c0cc4030c31d2d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 1 Nov 2025 07:11:49 +0000 Subject: [PATCH 1/7] Initial plan From 086f16f555b989ebb2141f6e3621dd5b3ae0abaa Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 1 Nov 2025 07:17:30 +0000 Subject: [PATCH 2/7] Fix hopper task restart bug and improve thread-safety - Fix bug where hopper tasks would stop after one execution by ensuring proper task restart in startHopperTask - Wrap restartHopperForSpawner in location-based scheduler for Folia thread-safety - Add atomic task replacement: stopHopperTask before starting new one to prevent race conditions - Add comprehensive null checks throughout all hopper methods - Improve error handling with try-catch blocks and detailed logging - Optimize cleanup method to safely cancel all tasks - Add validation checks in processChunkHoppers and onChunkUnload - Ensure thread-safe access to activeHoppers ConcurrentHashMap Co-authored-by: ptthanh02 <73684260+ptthanh02@users.noreply.github.com> --- .../smartspawner/extras/HopperHandler.java | 121 +++++++++++++----- 1 file changed, 89 insertions(+), 32 deletions(-) diff --git a/core/src/main/java/github/nighter/smartspawner/extras/HopperHandler.java b/core/src/main/java/github/nighter/smartspawner/extras/HopperHandler.java index 2bbb5faa..70c1d88d 100644 --- a/core/src/main/java/github/nighter/smartspawner/extras/HopperHandler.java +++ b/core/src/main/java/github/nighter/smartspawner/extras/HopperHandler.java @@ -72,15 +72,23 @@ public void restartAllHoppers() { } private void processChunkHoppers(Chunk chunk) { + if (chunk == null || !chunk.isLoaded()) return; + Location chunkLoc = new Location(chunk.getWorld(), chunk.getX() * 16 + 8, 64, chunk.getZ() * 16 + 8); Scheduler.runLocationTask(chunkLoc, () -> { try { - for (BlockState state : chunk.getTileEntities(block -> block.getType() == Material.HOPPER, false)) { + BlockState[] tileEntities = chunk.getTileEntities(block -> block.getType() == Material.HOPPER, false); + if (tileEntities == null || tileEntities.length == 0) return; + + for (BlockState state : tileEntities) { + if (state == null) continue; + Block hopperBlock = state.getBlock(); - Block aboveBlock = hopperBlock.getRelative(BlockFace.UP); + if (hopperBlock.getType() != Material.HOPPER) continue; + Block aboveBlock = hopperBlock.getRelative(BlockFace.UP); if (aboveBlock.getType() == Material.SPAWNER) { startHopperTask(hopperBlock.getLocation(), aboveBlock.getLocation()); } @@ -102,15 +110,42 @@ public void onChunkLoad(ChunkLoadEvent event) { @EventHandler public void onChunkUnload(ChunkUnloadEvent event) { + if (!plugin.getConfig().getBoolean("hopper.enabled", false)) return; + Chunk chunk = event.getChunk(); - for (BlockState state : chunk.getTileEntities(block -> block.getType() == Material.HOPPER, false)) { - stopHopperTask(state.getLocation()); + if (chunk == null) return; + + try { + BlockState[] tileEntities = chunk.getTileEntities(block -> block.getType() == Material.HOPPER, false); + if (tileEntities == null || tileEntities.length == 0) return; + + for (BlockState state : tileEntities) { + if (state == null) continue; + stopHopperTask(state.getLocation()); + } + } catch (Exception e) { + plugin.getLogger().log(Level.WARNING, "Error stopping hoppers in unloading chunk at " + + chunk.getX() + "," + chunk.getZ(), e); } } public void cleanup() { - activeHoppers.values().forEach(Scheduler.Task::cancel); - activeHoppers.clear(); + try { + List tasks = new ArrayList<>(activeHoppers.values()); + activeHoppers.clear(); + + for (Scheduler.Task task : tasks) { + if (task != null) { + try { + task.cancel(); + } catch (Exception e) { + plugin.getLogger().log(Level.WARNING, "Error cancelling hopper task during cleanup", e); + } + } + } + } catch (Exception e) { + plugin.getLogger().log(Level.SEVERE, "Error during hopper handler cleanup", e); + } } @EventHandler @@ -133,9 +168,10 @@ public void onHopperBreak(BlockBreakEvent event) { public void startHopperTask(Location hopperLoc, Location spawnerLoc) { if (!plugin.getConfig().getBoolean("hopper.enabled", false)) return; - if (activeHoppers.containsKey(hopperLoc)) return; + if (hopperLoc == null || spawnerLoc == null) return; + + stopHopperTask(hopperLoc); - // Create a runnable for the hopper task Runnable hopperRunnable = () -> { try { if (!isValidSetup(hopperLoc, spawnerLoc)) { @@ -145,20 +181,21 @@ public void startHopperTask(Location hopperLoc, Location spawnerLoc) { transferItems(hopperLoc, spawnerLoc); } catch (Exception e) { plugin.getLogger().log(Level.WARNING, "Error in hopper task at " + hopperLoc, e); - // Don't stop the task on error, just log it } }; - // Use the location-based scheduler for better Folia compatibility try { + long checkDelay = plugin.getTimeFromConfig("hopper.check_delay", "3s"); Scheduler.Task task = Scheduler.runLocationTaskTimer( hopperLoc, hopperRunnable, 0L, - plugin.getTimeFromConfig("hopper.check_delay", "3s") + checkDelay ); - activeHoppers.put(hopperLoc, task); + if (task != null && !task.isCancelled()) { + activeHoppers.put(hopperLoc, task); + } } catch (Exception e) { plugin.getLogger().log(Level.SEVERE, "Failed to start hopper task at " + hopperLoc, e); } @@ -174,48 +211,68 @@ private boolean isValidSetup(Location hopperLoc, Location spawnerLoc) { } public void stopHopperTask(Location hopperLoc) { + if (hopperLoc == null) return; + Scheduler.Task task = activeHoppers.remove(hopperLoc); if (task != null) { - task.cancel(); + try { + task.cancel(); + } catch (Exception e) { + plugin.getLogger().log(Level.WARNING, "Error cancelling hopper task at " + hopperLoc, e); + } } } public void restartHopperForSpawner(Location spawnerLoc) { if (!plugin.getConfig().getBoolean("hopper.enabled", false)) return; - - // Find hopper below the spawner - Block spawnerBlock = spawnerLoc.getBlock(); - if (spawnerBlock.getType() != Material.SPAWNER) return; - - Block hopperBlock = spawnerBlock.getRelative(BlockFace.DOWN); - if (hopperBlock.getType() != Material.HOPPER) return; - - Location hopperLoc = hopperBlock.getLocation(); - - // Stop existing hopper task if any - stopHopperTask(hopperLoc); - - // Start new hopper task - startHopperTask(hopperLoc, spawnerLoc); + if (spawnerLoc == null) return; + + Scheduler.runLocationTask(spawnerLoc, () -> { + try { + Block spawnerBlock = spawnerLoc.getBlock(); + if (spawnerBlock.getType() != Material.SPAWNER) return; + + Block hopperBlock = spawnerBlock.getRelative(BlockFace.DOWN); + if (hopperBlock.getType() != Material.HOPPER) return; + + Location hopperLoc = hopperBlock.getLocation(); + + startHopperTask(hopperLoc, spawnerLoc); + } catch (Exception e) { + plugin.getLogger().log(Level.WARNING, "Error restarting hopper for spawner at " + spawnerLoc, e); + } + }); } private void transferItems(Location hopperLoc, Location spawnerLoc) { + if (hopperLoc == null || spawnerLoc == null) return; + SpawnerData spawner = spawnerManager.getSpawnerByLocation(spawnerLoc); if (spawner == null) return; - // Use inventoryLock for hopper transfers ReentrantLock lock = spawner.getInventoryLock(); - if (!lock.tryLock()) return; // Skip this tick if we can't get the lock + if (!lock.tryLock()) return; try { + Block hopperBlock = hopperLoc.getBlock(); + if (hopperBlock.getType() != Material.HOPPER) { + stopHopperTask(hopperLoc); + return; + } + VirtualInventory virtualInv = spawner.getVirtualInventory(); - Hopper hopper = (Hopper) hopperLoc.getBlock().getState(false); + if (virtualInv == null) return; + + Hopper hopper = (Hopper) hopperBlock.getState(false); + if (hopper == null) return; int itemsPerTransfer = plugin.getConfig().getInt("hopper.stack_per_transfer", 5); int transferred = 0; boolean inventoryChanged = false; Map displayItems = virtualInv.getDisplayInventory(); + if (displayItems == null || displayItems.isEmpty()) return; + List itemsToRemove = new ArrayList<>(); for (Map.Entry entry : displayItems.entrySet()) { @@ -261,7 +318,7 @@ private void transferItems(Location hopperLoc, Location spawnerLoc) { updateOpenGuis(spawner); } } catch (Exception e) { - plugin.getLogger().log(Level.WARNING, "Error transferring items from spawner to hopper", e); + plugin.getLogger().log(Level.WARNING, "Error transferring items from spawner to hopper at " + hopperLoc, e); } finally { lock.unlock(); } From 348a933b9caf276033cb0f1d507d33bedf1cab09 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 1 Nov 2025 07:18:54 +0000 Subject: [PATCH 3/7] Ensure proper cleanup when reloading hopper handler - Add cleanup of old HopperHandler before creating new one in setUpHopperHandler - Prevents resource leaks and duplicate tasks when config is reloaded - Ensures all old hopper tasks are properly cancelled before restart Co-authored-by: ptthanh02 <73684260+ptthanh02@users.noreply.github.com> --- .../java/github/nighter/smartspawner/SmartSpawner.java | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/github/nighter/smartspawner/SmartSpawner.java b/core/src/main/java/github/nighter/smartspawner/SmartSpawner.java index a757e1f4..46b06d9f 100644 --- a/core/src/main/java/github/nighter/smartspawner/SmartSpawner.java +++ b/core/src/main/java/github/nighter/smartspawner/SmartSpawner.java @@ -311,7 +311,14 @@ private void initializeListeners() { } public void setUpHopperHandler() { - this.hopperHandler = getConfig().getBoolean("hopper.enabled", false) ? new HopperHandler(this) : null; + if (this.hopperHandler != null) { + this.hopperHandler.cleanup(); + this.hopperHandler = null; + } + + if (getConfig().getBoolean("hopper.enabled", false)) { + this.hopperHandler = new HopperHandler(this); + } } private void registerListeners() { From c71d66559dcdd8b86c66c22f7d69c104189b37fe Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 1 Nov 2025 07:20:50 +0000 Subject: [PATCH 4/7] Address code review feedback for improved readability - Rename tileEntities to hopperStates for better clarity - Inline checkDelay variable for better readability Co-authored-by: ptthanh02 <73684260+ptthanh02@users.noreply.github.com> --- .../smartspawner/extras/HopperHandler.java | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/core/src/main/java/github/nighter/smartspawner/extras/HopperHandler.java b/core/src/main/java/github/nighter/smartspawner/extras/HopperHandler.java index 70c1d88d..6e971385 100644 --- a/core/src/main/java/github/nighter/smartspawner/extras/HopperHandler.java +++ b/core/src/main/java/github/nighter/smartspawner/extras/HopperHandler.java @@ -79,10 +79,10 @@ private void processChunkHoppers(Chunk chunk) { Scheduler.runLocationTask(chunkLoc, () -> { try { - BlockState[] tileEntities = chunk.getTileEntities(block -> block.getType() == Material.HOPPER, false); - if (tileEntities == null || tileEntities.length == 0) return; + BlockState[] hopperStates = chunk.getTileEntities(block -> block.getType() == Material.HOPPER, false); + if (hopperStates == null || hopperStates.length == 0) return; - for (BlockState state : tileEntities) { + for (BlockState state : hopperStates) { if (state == null) continue; Block hopperBlock = state.getBlock(); @@ -116,10 +116,10 @@ public void onChunkUnload(ChunkUnloadEvent event) { if (chunk == null) return; try { - BlockState[] tileEntities = chunk.getTileEntities(block -> block.getType() == Material.HOPPER, false); - if (tileEntities == null || tileEntities.length == 0) return; + BlockState[] hopperStates = chunk.getTileEntities(block -> block.getType() == Material.HOPPER, false); + if (hopperStates == null || hopperStates.length == 0) return; - for (BlockState state : tileEntities) { + for (BlockState state : hopperStates) { if (state == null) continue; stopHopperTask(state.getLocation()); } @@ -185,12 +185,11 @@ public void startHopperTask(Location hopperLoc, Location spawnerLoc) { }; try { - long checkDelay = plugin.getTimeFromConfig("hopper.check_delay", "3s"); Scheduler.Task task = Scheduler.runLocationTaskTimer( hopperLoc, hopperRunnable, 0L, - checkDelay + plugin.getTimeFromConfig("hopper.check_delay", "3s") ); if (task != null && !task.isCancelled()) { From b64fc4ee89b76e3d0f5d5238d7fc352aba0a2c20 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 1 Nov 2025 07:27:19 +0000 Subject: [PATCH 5/7] Fix compilation error: use Collection instead of array for getTileEntities - Change BlockState[] to Collection for getTileEntities return type - Replace .length check with .isEmpty() for Collection - Fixes incompatible types compilation error in processChunkHoppers and onChunkUnload Co-authored-by: ptthanh02 <73684260+ptthanh02@users.noreply.github.com> --- .../github/nighter/smartspawner/extras/HopperHandler.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/github/nighter/smartspawner/extras/HopperHandler.java b/core/src/main/java/github/nighter/smartspawner/extras/HopperHandler.java index 6e971385..8ee3019d 100644 --- a/core/src/main/java/github/nighter/smartspawner/extras/HopperHandler.java +++ b/core/src/main/java/github/nighter/smartspawner/extras/HopperHandler.java @@ -79,8 +79,8 @@ private void processChunkHoppers(Chunk chunk) { Scheduler.runLocationTask(chunkLoc, () -> { try { - BlockState[] hopperStates = chunk.getTileEntities(block -> block.getType() == Material.HOPPER, false); - if (hopperStates == null || hopperStates.length == 0) return; + Collection hopperStates = chunk.getTileEntities(block -> block.getType() == Material.HOPPER, false); + if (hopperStates == null || hopperStates.isEmpty()) return; for (BlockState state : hopperStates) { if (state == null) continue; @@ -116,8 +116,8 @@ public void onChunkUnload(ChunkUnloadEvent event) { if (chunk == null) return; try { - BlockState[] hopperStates = chunk.getTileEntities(block -> block.getType() == Material.HOPPER, false); - if (hopperStates == null || hopperStates.length == 0) return; + Collection hopperStates = chunk.getTileEntities(block -> block.getType() == Material.HOPPER, false); + if (hopperStates == null || hopperStates.isEmpty()) return; for (BlockState state : hopperStates) { if (state == null) continue; From 3734aa0b21cf943cac4215c80a394675b1f528cd Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 1 Nov 2025 08:11:22 +0000 Subject: [PATCH 6/7] Optimize transferItems() for better performance Performance improvements: - Cache hopper inventory and contents outside loops to avoid repeated getContents() calls - Pre-allocate ArrayList with expected size to reduce resizing - Use itemTransferred flag to exit inner loop early instead of break - Cache currentAmount and maxStackSize to reduce method calls - Separate isSimilar check from amount check for better readability - Update hopperContents array after modification to keep cache synchronized These changes reduce unnecessary object allocations and method calls in the hot path, improving performance especially when processing multiple items per transfer cycle. Co-authored-by: ptthanh02 <73684260+ptthanh02@users.noreply.github.com> --- .../smartspawner/extras/HopperHandler.java | 57 +++++++++++-------- 1 file changed, 32 insertions(+), 25 deletions(-) diff --git a/core/src/main/java/github/nighter/smartspawner/extras/HopperHandler.java b/core/src/main/java/github/nighter/smartspawner/extras/HopperHandler.java index 8ee3019d..748bffda 100644 --- a/core/src/main/java/github/nighter/smartspawner/extras/HopperHandler.java +++ b/core/src/main/java/github/nighter/smartspawner/extras/HopperHandler.java @@ -265,14 +265,17 @@ private void transferItems(Location hopperLoc, Location spawnerLoc) { Hopper hopper = (Hopper) hopperBlock.getState(false); if (hopper == null) return; - int itemsPerTransfer = plugin.getConfig().getInt("hopper.stack_per_transfer", 5); - int transferred = 0; - boolean inventoryChanged = false; - Map displayItems = virtualInv.getDisplayInventory(); if (displayItems == null || displayItems.isEmpty()) return; - List itemsToRemove = new ArrayList<>(); + int itemsPerTransfer = plugin.getConfig().getInt("hopper.stack_per_transfer", 5); + + org.bukkit.inventory.Inventory hopperInv = hopper.getInventory(); + ItemStack[] hopperContents = hopperInv.getContents(); + + List itemsToRemove = new ArrayList<>(itemsPerTransfer); + int transferred = 0; + boolean inventoryChanged = false; for (Map.Entry entry : displayItems.entrySet()) { if (transferred >= itemsPerTransfer) break; @@ -280,31 +283,35 @@ private void transferItems(Location hopperLoc, Location spawnerLoc) { ItemStack item = entry.getValue(); if (item == null || item.getType() == Material.AIR) continue; - ItemStack[] hopperContents = hopper.getInventory().getContents(); - for (int i = 0; i < hopperContents.length; i++) { - if (transferred >= itemsPerTransfer) break; - + boolean itemTransferred = false; + for (int i = 0; i < hopperContents.length && !itemTransferred; i++) { ItemStack hopperItem = hopperContents[i]; + if (hopperItem == null || hopperItem.getType() == Material.AIR) { - hopper.getInventory().setItem(i, item.clone()); + hopperInv.setItem(i, item.clone()); + hopperContents[i] = hopperInv.getItem(i); itemsToRemove.add(item); transferred++; inventoryChanged = true; - break; - } else if (hopperItem.isSimilar(item) && - hopperItem.getAmount() < hopperItem.getMaxStackSize()) { - int space = hopperItem.getMaxStackSize() - hopperItem.getAmount(); - int toTransfer = Math.min(space, item.getAmount()); - - hopperItem.setAmount(hopperItem.getAmount() + toTransfer); - - ItemStack toRemove = item.clone(); - toRemove.setAmount(toTransfer); - itemsToRemove.add(toRemove); - - transferred++; - inventoryChanged = true; - break; + itemTransferred = true; + } else if (hopperItem.isSimilar(item)) { + int currentAmount = hopperItem.getAmount(); + int maxStackSize = hopperItem.getMaxStackSize(); + + if (currentAmount < maxStackSize) { + int space = maxStackSize - currentAmount; + int toTransfer = Math.min(space, item.getAmount()); + + hopperItem.setAmount(currentAmount + toTransfer); + + ItemStack toRemove = item.clone(); + toRemove.setAmount(toTransfer); + itemsToRemove.add(toRemove); + + transferred++; + inventoryChanged = true; + itemTransferred = true; + } } } } From 01980fe7ade464a9e00615759c0e5c47e8790565 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 1 Nov 2025 08:14:57 +0000 Subject: [PATCH 7/7] Fix potential exploit by refreshing hopper contents per item Moved getContents() call inside the outer loop (per spawner item) instead of caching it for the entire transfer operation. This prevents exploits where players could interact with the hopper during transfers, as we now get fresh inventory state for each item being transferred. Balance between performance and security: - Still cache hopperInv reference to avoid repeated getInventory() calls - Get fresh contents for each spawner item (not each hopper slot) - Removed stale cache update line that's no longer needed Addresses security concern while maintaining most performance improvements. Co-authored-by: ptthanh02 <73684260+ptthanh02@users.noreply.github.com> --- .../github/nighter/smartspawner/extras/HopperHandler.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/github/nighter/smartspawner/extras/HopperHandler.java b/core/src/main/java/github/nighter/smartspawner/extras/HopperHandler.java index 748bffda..e57581a7 100644 --- a/core/src/main/java/github/nighter/smartspawner/extras/HopperHandler.java +++ b/core/src/main/java/github/nighter/smartspawner/extras/HopperHandler.java @@ -271,7 +271,6 @@ private void transferItems(Location hopperLoc, Location spawnerLoc) { int itemsPerTransfer = plugin.getConfig().getInt("hopper.stack_per_transfer", 5); org.bukkit.inventory.Inventory hopperInv = hopper.getInventory(); - ItemStack[] hopperContents = hopperInv.getContents(); List itemsToRemove = new ArrayList<>(itemsPerTransfer); int transferred = 0; @@ -283,13 +282,14 @@ private void transferItems(Location hopperLoc, Location spawnerLoc) { ItemStack item = entry.getValue(); if (item == null || item.getType() == Material.AIR) continue; + ItemStack[] hopperContents = hopperInv.getContents(); + boolean itemTransferred = false; for (int i = 0; i < hopperContents.length && !itemTransferred; i++) { ItemStack hopperItem = hopperContents[i]; if (hopperItem == null || hopperItem.getType() == Material.AIR) { hopperInv.setItem(i, item.clone()); - hopperContents[i] = hopperInv.getItem(i); itemsToRemove.add(item); transferred++; inventoryChanged = true;