Fix: backpack issues #1183

m1919810 · 2026-01-07T06:42:33Z

简介

Pull request overview

This PR refactors the backpack system to address issues related to #1181. The refactoring focuses on improving concurrency handling, introducing a snapshot-based change tracking mechanism, and modernizing the async callback pattern using CompletableFuture.

Introduces ThreadUtils utility for safer main thread execution using NMS reflection with fallback
Refactors backpack inventory tracking using InvSnapshot for better change detection
Replaces callback-based async operations with CompletableFuture for cleaner async code

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 17 comments.

Show a summary per file

File	Description
ThreadUtils.java	New utility class for thread-safe main thread execution with NMS reflection fallback
BackpackListener.java	Major refactor of backpack open/close logic with improved tracking and validation
PlayerBackpack.java	Adds snapshot mechanism and CompletableFuture-based async API
InvSnapshot.java	New immutable snapshot class for inventory state tracking
ProfileDataController.java	Refactors saveBackpackInventory to compute changed slots internally with synchronization
AbstractCraftingTable.java	Simplifies backpack upgrade logic using new CompletableFuture API
CoolerListener.java	Updates saveBackpackInventory call to remove slot parameter
InvStorageUtils.java	Adds overload to accept InvSnapshot directly
BackpackCache.java	Changes visibility from package-private to public
BlockDataController.java	Updates to use InvSnapshot class instead of raw List
PlayerProfileMigrator.java	Updates saveBackpackInventory call in migration code
ADataController.java	Adds @Getter annotations for executor access
IAsyncReadCallback.java	Marks interface as deprecated

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-07T06:50:53Z

src/main/java/io/github/thebusybiscuit/slimefun4/implementation/listeners/BackpackListener.java

+        //        } else {
+        //            Slimefun.getLocalization().sendMessage(p, "backpack.already-open", true);
+        //        }


The commented-out code should be removed. Lines 262-264 contain leftover code that serves no purpose and reduces code readability. If this is related to the "already-open" check logic, it should either be properly implemented or completely removed.

Suggested change

// } else {

// Slimefun.getLocalization().sendMessage(p, "backpack.already-open", true);

// }

Copilot · 2026-01-07T06:50:53Z

src/main/java/io/github/thebusybiscuit/slimefun4/implementation/listeners/BackpackListener.java

+                    return;
+                }
+                // Check if the item is modified during the chat input
+                if (!Objects.equals(itemCopy, item)) {


The item comparison on line 204 uses Objects.equals which checks for exact object equality. However, the comparison should check if the ItemStack has been modified in terms of its properties (type, amount, metadata), not object reference. Use ItemStack.equals() or ItemStack.isSimilar() instead for proper item comparison.

Suggested change

if (!Objects.equals(itemCopy, item)) {

if (!itemCopy.equals(item)) {

Copilot · 2026-01-07T06:50:54Z

src/main/java/io/github/thebusybiscuit/slimefun4/implementation/listeners/BackpackListener.java

+            if (invSnapshot.remove(backpack.getUniqueId()) == null) {
+                reportBackpackInvCacheError(p, backpack, null);
+            }
+            // The changedSlot computation and refreshSnapshot is moved to the
+            // ProfileDataController#saveBackpackInventory
+            Slimefun.getDatabaseManager().getProfileDataController().saveBackpackInventory(backpack);


Missing null check for InvSnapshot. The code removes the InvSnapshot from the map (line 68) and checks if it's null (line 68-70), but then on line 73 calls saveBackpackInventory without ensuring that the snapshot exists in the PlayerBackpack. If the snapshot was null and reported as an error, the save operation should probably be handled differently or skipped.

Copilot · 2026-01-07T06:50:54Z

src/main/java/io/github/thebusybiscuit/slimefun4/utils/ThreadUtils.java

+     * If the input is already a FutureTask, it is cast and returned.
+     * Otherwise, a new FutureTask is created with the Runnable and null result.
+     *
+     * @param runnable The runnable to wrap
+     * @return A FutureTask wrapping the runnable
+     */
+    public static FutureTask<Void> getFutureTask(Runnable runnable) {
+        return runnable instanceof FutureTask<?> future
+                ? (FutureTask<Void>) future
+                : new FutureTask<>(runnable, (Void) null);


Potential unsafe cast. On line 74, the code casts FutureTask<?> to FutureTask<Void>, but if the FutureTask was created with a different result type, this cast could lead to ClassCastException when the result is retrieved. Consider checking the result type or documenting that this method should only be called with Runnable-based FutureTasks that return Void.

Suggested change

* If the input is already a FutureTask, it is cast and returned.

* Otherwise, a new FutureTask is created with the Runnable and null result.

*

* @param runnable The runnable to wrap

* @return A FutureTask wrapping the runnable

*/

public static FutureTask<Void> getFutureTask(Runnable runnable) {

return runnable instanceof FutureTask<?> future

? (FutureTask<Void>) future

: new FutureTask<>(runnable, (Void) null);

* If the input is already a FutureTask, it is returned as-is.

* Otherwise, a new FutureTask is created with the Runnable and a null result.

*

* @param runnable The runnable to wrap

* @return A FutureTask wrapping the runnable

*/

public static FutureTask<?> getFutureTask(Runnable runnable) {

return runnable instanceof FutureTask<?> future

? future

: new FutureTask<>(runnable, null);

Copilot · 2026-01-07T06:50:54Z

src/main/java/io/github/thebusybiscuit/slimefun4/api/player/PlayerBackpack.java

+    /**
+     * This refreshes the internal snapshot,
+     * It should be called after every database writing task
+     * It should not be called elsewhere
+     */
+    public void refreshSnapshot() {
+        this.snapshot = new InvSnapshot(inventory);
    }


Missing documentation for the refreshSnapshot method. While there is a comment (lines 257-261), it lacks proper JavaDoc format and doesn't clearly explain the threading requirements, synchronization guarantees, or what happens if it's called at the wrong time. Consider converting to proper JavaDoc with @throws or @implNote tags as appropriate.

Copilot · 2026-01-07T06:50:56Z

src/main/java/com/xzavier0722/mc/plugin/slimefun4/storage/controller/ProfileDataController.java

+    @Deprecated(forRemoval = true)
+    public void saveBackpackInventory(PlayerBackpack bp, Set<Integer> slotsIgnored) {
+        // we decided to compute slots internal, and the argument is ignored to avoid potential data desync
+        saveBackpackInventory(bp);
+    }
+
+    public void saveBackpackInventory(@Nonnull PlayerBackpack bp) {
+        // avoid asynchronize save
+        synchronized (bp) {
+            Set<Integer> slots = bp.getSnapshot().getChangedSlots(bp.getInventory());
+            bp.refreshSnapshot();
+            var id = bp.getUniqueId().toString();
+            var inv = bp.getInventory();
+            slots.forEach(slot -> {
+                var key = new RecordKey(DataScope.BACKPACK_INVENTORY);
+                key.addCondition(FieldKey.BACKPACK_ID, id);
+                key.addCondition(FieldKey.INVENTORY_SLOT, slot + "");
+                key.addField(FieldKey.INVENTORY_ITEM);
+                var is = inv.getItem(slot);
+                if (is == null) {
+                    scheduleDeleteTask(new UUIDKey(DataScope.NONE, bp.getOwner().getUniqueId()), key, false);
+                } else {
+                    try {
+                        var data = new RecordSet();
+                        data.put(FieldKey.BACKPACK_ID, id);
+                        data.put(FieldKey.INVENTORY_SLOT, slot + "");
+                        data.put(FieldKey.INVENTORY_ITEM, is);
+                        scheduleWriteTask(
+                                new UUIDKey(DataScope.NONE, bp.getOwner().getUniqueId()), key, data, false);
+                    } catch (IllegalArgumentException e) {
+                        Slimefun.logger().log(Level.WARNING, e.getMessage());
+                    }
                }
-            }
-        });
+            });
+        }
    }

+    @Deprecated(forRemoval = true)
    public void saveBackpackInventory(PlayerBackpack bp, Integer... slots) {
-        saveBackpackInventory(bp, Set.of(slots));
+        // we decided to compute slots internal, and the argument is ignored to avoid potential data desync
+        saveBackpackInventory(bp);
    }


Unclear deprecation reasoning. The deprecation annotations on lines 345 and 382 state "forRemoval = true" but the implementation comment says the argument is "ignored to avoid potential data desync." This is confusing - if the method is kept for backward compatibility but behaves differently than expected, this could break existing code silently. Consider either removing these methods entirely or documenting the behavioral change more clearly in the deprecation message.

Copilot · 2026-01-07T06:50:56Z

src/main/java/io/github/thebusybiscuit/slimefun4/implementation/listeners/BackpackListener.java

+                            // Check if someone else is currently viewing this backpack
+                            if (backpacks.containsValue(bp.getUniqueId())
+                                    || !bp.getInventory().getViewers().isEmpty()) {
+                                Slimefun.getLocalization().sendMessage(p, "backpack.already-open", true);
+                                return;
+                            }
+                            // If backpacks doesn't contain the bp uniqueId, then the existence of bp's invSnapshot must
+                            // be a bug
+                            if (invSnapshot.containsKey(bp.getUniqueId())) {
+                                Slimefun.getLocalization().sendMessage(p, "backpack.already-open", true);
+                                reportBackpackInvCacheError(p, bp, invSnapshot.remove(bp.getUniqueId()));
+                                return;
+                            }
+                            SoundEffect.BACKPACK_OPEN_SOUND.playAt(p.getLocation(), SoundCategory.PLAYERS);
+                            bp.open(p);
+                            backpacks.put(p.getUniqueId(), bp.getUniqueId());
+                            backpackInstances.put(p.getUniqueId(), backpackItem);
+                            // create a InvSnapshot just for fun (bs
+                            // should not call bp.refreshSnapshot()
+                            invSnapshot.put(bp.getUniqueId(), new InvSnapshot(bp.getInventory()));


Potential race condition in backpack opening logic. Between the check on line 241-242 (checking if backpack is already open) and line 255 (adding to backpacks map), another thread could open the same backpack, resulting in multiple viewers. Consider moving the check and map insertion into a synchronized block or using a ConcurrentHashMap with putIfAbsent to ensure atomicity.

Suggested change

// Check if someone else is currently viewing this backpack

if (backpacks.containsValue(bp.getUniqueId())

|| !bp.getInventory().getViewers().isEmpty()) {

Slimefun.getLocalization().sendMessage(p, "backpack.already-open", true);

return;

}

// If backpacks doesn't contain the bp uniqueId, then the existence of bp's invSnapshot must

// be a bug

if (invSnapshot.containsKey(bp.getUniqueId())) {

Slimefun.getLocalization().sendMessage(p, "backpack.already-open", true);

reportBackpackInvCacheError(p, bp, invSnapshot.remove(bp.getUniqueId()));

return;

}

SoundEffect.BACKPACK_OPEN_SOUND.playAt(p.getLocation(), SoundCategory.PLAYERS);

bp.open(p);

backpacks.put(p.getUniqueId(), bp.getUniqueId());

backpackInstances.put(p.getUniqueId(), backpackItem);

// create a InvSnapshot just for fun (bs

// should not call bp.refreshSnapshot()

invSnapshot.put(bp.getUniqueId(), new InvSnapshot(bp.getInventory()));

synchronized (backpacks) {

// Check if someone else is currently viewing this backpack

if (backpacks.containsValue(bp.getUniqueId())

|| !bp.getInventory().getViewers().isEmpty()) {

Slimefun.getLocalization().sendMessage(p, "backpack.already-open", true);

return;

}

// If backpacks doesn't contain the bp uniqueId, then the existence of bp's invSnapshot must

// be a bug

if (invSnapshot.containsKey(bp.getUniqueId())) {

Slimefun.getLocalization().sendMessage(p, "backpack.already-open", true);

reportBackpackInvCacheError(p, bp, invSnapshot.remove(bp.getUniqueId()));

return;

}

SoundEffect.BACKPACK_OPEN_SOUND.playAt(p.getLocation(), SoundCategory.PLAYERS);

bp.open(p);

backpacks.put(p.getUniqueId(), bp.getUniqueId());

backpackInstances.put(p.getUniqueId(), backpackItem);

// create a InvSnapshot just for fun (bs

// should not call bp.refreshSnapshot()

invSnapshot.put(bp.getUniqueId(), new InvSnapshot(bp.getInventory()));

}

Copilot · 2026-01-07T06:50:57Z

src/main/java/io/github/thebusybiscuit/slimefun4/api/player/PlayerBackpack.java

+    @Nonnull
+    @Getter
+    private InvSnapshot snapshot;


The snapshot field is marked as @nonnull (line 56) but is not initialized in the field declaration. While it is initialized in the constructor (line 254), there could be a brief moment where the field is null if accessed before initialization completes, especially in a concurrent environment. Consider initializing the field inline or using a factory method pattern to ensure the object is fully constructed before being shared.

Copilot · 2026-01-07T06:50:57Z

src/main/java/com/xzavier0722/mc/plugin/slimefun4/storage/util/InvSnapshot.java

+import org.bukkit.inventory.Inventory;
+import org.bukkit.inventory.ItemStack;
+
+@Getter


Missing documentation for public class. The InvSnapshot class is a new public class but lacks class-level JavaDoc documentation. Add documentation explaining what an InvSnapshot is, when it should be used, its immutability guarantees, and typical usage patterns.

Suggested change

@Getter

@Getter

/**

* Immutable snapshot of an inventory's contents.

* 

* An {@code InvSnapshot} captures the state of an {@link Inventory} (or a raw

* {@link ItemStack} array) at a specific point in time. The underlying list of

* item/slot pairs is defensively copied on construction and cannot be modified

* afterwards, ensuring that the snapshot remains stable even if the original

* inventory or array is later changed.

* 

* 

* This class is typically used to detect which slots have changed between two

* inventory states. A common pattern is:

* 

* <pre>{@code

* InvSnapshot before = new InvSnapshot(inventory);

* // ... mutate the inventory ...

* Set<Integer> changedSlots = before.getChangedSlots(inventory);

* }</pre>

* 

* Use {@link #getChangedSlots(ItemStack[])} or {@link #getChangedSlots(Inventory)}

* to compare the stored snapshot against a current inventory view and obtain the

* set of slot indices whose contents differ.

* 

*/

Copilot · 2026-01-07T06:50:57Z

src/main/java/io/github/thebusybiscuit/slimefun4/api/player/PlayerBackpack.java

+
+                if (CommonPatterns.NUMERIC.matcher(splitLine[1]).matches()) {
+                    uuid = splitLine[0].replace(ChatColors.color("&7ID: "), "");
+                    id = OptionalInt.of(Integer.parseInt(splitLine[1]));


Potential uncaught 'java.lang.NumberFormatException'.

Suggested change

id = OptionalInt.of(Integer.parseInt(splitLine[1]));

try {

id = OptionalInt.of(Integer.parseInt(splitLine[1]));

} catch (NumberFormatException ignored) {

// Ignore invalid numeric values and leave 'id' empty

}

Copilot

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 8 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-07T08:16:17Z

src/main/java/io/github/thebusybiscuit/slimefun4/implementation/listeners/BackpackListener.java

                }
-                if (item.getAmount() > 1) {
-                    Slimefun.getLocalization().sendMessage(p, "backpack.no-stack", true);
+                // Check if the player moves the item


The comment contains a grammatical error. It should be "Check if the player moves the item" or "Check if the player moved the item".

src/main/java/com/xzavier0722/mc/plugin/slimefun4/storage/controller/ProfileDataController.java

Copilot · 2026-01-07T08:16:18Z

src/main/java/com/xzavier0722/mc/plugin/slimefun4/storage/callback/IAsyncReadCallback.java

+import java.util.function.Consumer;
+
+/**
+ * This interface is deprecated and related methods will be replaced by methods using {@link java.util.concurrent.CompletableFuture} in the future, <- this work is a completable future


The documentation contains a grammatical error. "this work is a completable future" should be "this work aims to replace them with CompletableFuture" or similar.

Suggested change

* This interface is deprecated and related methods will be replaced by methods using {@link java.util.concurrent.CompletableFuture} in the future, <- this work is a completable future

* This interface is deprecated and related methods will be replaced by methods using {@link java.util.concurrent.CompletableFuture} in the future; this work aims to replace them with {@link java.util.concurrent.CompletableFuture}.

Copilot · 2026-01-07T08:16:18Z

src/main/java/io/github/thebusybiscuit/slimefun4/implementation/listeners/BackpackListener.java

+    private final Map<UUID, UUID> backpacks = new HashMap<>();
+    private final Map<UUID, SlimefunBackpack> backpackInstances = new HashMap<>();
+    private final Map<UUID, InvSnapshot> invSnapshot = new HashMap<>();


The maps (backpacks, backpackInstances, invSnapshot) are not thread-safe. Since they can be accessed from multiple threads (main thread for events and async threads for database operations), using HashMap instead of ConcurrentHashMap can lead to race conditions and potential data corruption. Consider using ConcurrentHashMap for thread-safety.

Copilot · 2026-01-07T08:16:19Z

src/main/java/io/github/thebusybiscuit/slimefun4/implementation/listeners/BackpackListener.java

+                }
+                // Check if the item is modified during the chat input
+                if (!Objects.equals(itemCopy, item)) {
                    Slimefun.getLocalization().sendMessage(p, "backpack.not-original-item", true);


The error message is being sent to the original player 'p' instead of the resolved 'player' variable. Since the player reference has been resolved and null-checked at line 195-196, messages should be sent to 'player' to ensure consistency.

Copilot · 2026-01-07T08:16:19Z

src/main/java/io/github/thebusybiscuit/slimefun4/implementation/listeners/BackpackListener.java

+                Player player = Bukkit.getPlayer(puuid);
+                if (player == null) return;
+                var pInv = player.getInventory();
+                // Check if the player change the amount of item


The comment contains a grammatical error. It should be "Check if the player changes the amount of item" or "Check if the player changed the amount of item".

Suggested change

// Check if the player change the amount of item

// Check if the player changes the amount of the item

Copilot · 2026-01-07T08:16:19Z

.../github/thebusybiscuit/slimefun4/implementation/items/multiblocks/AbstractCraftingTable.java

-        if (backpack == null) {
-            return Optional.empty();
-        }
+                            onReadyCb.run();


The callback is executed unconditionally even when result is null. This means if the backpack retrieval fails, the callback still runs, potentially causing issues. Consider only running the callback when the operation succeeds, or handle the null case appropriately in the callback.

Copilot · 2026-01-07T08:16:20Z

src/main/java/com/xzavier0722/mc/plugin/slimefun4/storage/migrator/PlayerProfileMigrator.java

+            //            var changedSlot = new HashSet<Integer>();

            for (String key : configFile.getKeys("backpacks." + bpID + ".contents")) {
                var bpKey = Integer.parseInt(key);
                var item = configFile.getItem("backpacks." + bpID + ".contents." + bpKey);
                bp.getInventory().setItem(bpKey, item);
-                changedSlot.add(bpKey);
+                //                changedSlot.add(bpKey);


These commented-out lines should be removed rather than left in the codebase. If they're not needed anymore, they should be deleted. The commit history serves as a reference for previous implementations.

src/main/java/com/xzavier0722/mc/plugin/slimefun4/storage/callback/IAsyncReadCallback.java

Co-authored-by: Copilot <[email protected]>

StarWishsama · 2026-01-07T08:27:23Z

src/main/java/io/github/thebusybiscuit/slimefun4/implementation/listeners/BackpackListener.java

+                var pInv = player.getInventory();
+                // Check if the player change the amount of item
+                if (item.getAmount() != 1) {
+                    Slimefun.getLocalization().sendMessage(p, "backpack.no-stack", true);


Suggested change

Slimefun.getLocalization().sendMessage(p, "backpack.no-stack", true);

Slimefun.getLocalization().sendMessage(player, "backpack.no-stack", true);

StarWishsama · 2026-01-07T08:27:38Z

src/main/java/io/github/thebusybiscuit/slimefun4/implementation/listeners/BackpackListener.java

+                }
+                // Check if the item is modified during the chat input
+                if (!Objects.equals(itemCopy, item)) {
                    Slimefun.getLocalization().sendMessage(p, "backpack.not-original-item", true);


Suggested change

Slimefun.getLocalization().sendMessage(p, "backpack.not-original-item", true);

Slimefun.getLocalization().sendMessage(player, "backpack.not-original-item", true);

StarWishsama · 2026-01-07T08:27:51Z

src/main/java/io/github/thebusybiscuit/slimefun4/implementation/listeners/BackpackListener.java

-                    Slimefun.getLocalization().sendMessage(p, "backpack.no-stack", true);
+                // Check if the player moves the item
+                if (!item.equals(pInv.getItemInMainHand()) && !item.equals(pInv.getItemInOffHand())) {
+                    Slimefun.getLocalization().sendMessage(p, "backpack.not-original-item", true);


Suggested change

Slimefun.getLocalization().sendMessage(p, "backpack.not-original-item", true);

Slimefun.getLocalization().sendMessage(player, "backpack.not-original-item", true);

…un-vModified into fix/backpack_issues

Copilot

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 8 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-08T10:16:22Z

src/main/java/io/github/thebusybiscuit/slimefun4/api/player/PlayerBackpack.java

+        if (contents != null) {
+            if (size != contents.length) {
+                throw new IllegalArgumentException("Invalid contents: size mismatched!");
+            }
+            inventory.setContents(contents);
        }

-        if (size != contents.length) {
-            throw new IllegalArgumentException("Invalid contents: size mismatched!");
-        }
-        inventory.setContents(contents);
+        this.snapshot = new InvSnapshot(inventory);


When contents is null, the snapshot is created from an empty inventory. However, when contents is not null but the size doesn't match, an exception is thrown before the snapshot can be initialized. This leaves the @nonnull snapshot field uninitialized, which can cause NullPointerException later. The exception should be thrown before the super constructor call, or the snapshot should be initialized in a finally block or at the field level.

Copilot · 2026-01-08T10:16:22Z

src/main/java/com/xzavier0722/mc/plugin/slimefun4/storage/callback/IAsyncReadCallback.java

+ * This interface is deprecated and related methods will be replaced by methods using {@link java.util.concurrent.CompletableFuture} in the future, <- this work is a completable future
+ * related methods will be kept and mark deprecated
+ * you can run your original {@link IAsyncReadCallback} callbacks using {@link java.util.concurrent.CompletableFuture#thenAcceptAsync(Consumer, java.util.concurrent.Executor)}, don't forget to check whether the accepted value is null or not
+ * If you want to invoke callback not on the main thread, use
+ * {@link ADataController#getCallbackExecutor}
+ * If you want to invoke callback on the main thread , see
+ * {@link ThreadUtils#getMainThreadExecutor()} or {@link ThreadUtils#getMainDelayedExecutor()}
+ * @param <T>


The documentation comment contains a typo and grammatical error. The phrase "related methods will be replaced by methods using CompletableFuture in the future, <- this work is a completable future" is unclear. Consider revising to: "This interface is deprecated. Related methods are being replaced by methods using CompletableFuture."

Suggested change

* This interface is deprecated and related methods will be replaced by methods using {@link java.util.concurrent.CompletableFuture} in the future, <- this work is a completable future

* related methods will be kept and mark deprecated

* you can run your original {@link IAsyncReadCallback} callbacks using {@link java.util.concurrent.CompletableFuture#thenAcceptAsync(Consumer, java.util.concurrent.Executor)}, don't forget to check whether the accepted value is null or not

* If you want to invoke callback not on the main thread, use

* {@link ADataController#getCallbackExecutor}

* If you want to invoke callback on the main thread , see

* {@link ThreadUtils#getMainThreadExecutor()} or {@link ThreadUtils#getMainDelayedExecutor()}

* @param <T>

* This interface is deprecated. Related methods are being replaced by methods using {@link java.util.concurrent.CompletableFuture}.

* Existing related methods will be kept and marked as deprecated for backward compatibility.

* You can run your original {@link IAsyncReadCallback} callbacks using

* {@link java.util.concurrent.CompletableFuture#thenAcceptAsync(Consumer, java.util.concurrent.Executor)}; don't forget to check whether the accepted value is null or not.

* If you want to invoke the callback not on the main thread, use

* {@link ADataController#getCallbackExecutor}.

* If you want to invoke the callback on the main thread, see

* {@link ThreadUtils#getMainThreadExecutor()} or {@link ThreadUtils#getMainDelayedExecutor()}.

* @param <T> the type of the result

Copilot · 2026-01-08T10:16:22Z

src/main/java/com/xzavier0722/mc/plugin/slimefun4/storage/controller/ProfileDataController.java

+    @Deprecated(forRemoval = true)
+    public void saveBackpackInventory(PlayerBackpack bp, Set<Integer> slotsIgnored) {
+        // we decided to compute slots internal, and the argument is ignored to avoid potential data desync
+        saveBackpackInventory(bp);


The parameter name "slotsIgnored" accurately describes that the parameter is ignored, which is good. However, the method implementation now computes slots internally, which means calling code that relies on specifying exact slots will have different behavior. This is a breaking change in behavior, not just a deprecation. Consider documenting this behavioral change more prominently in the deprecation notice or providing a migration guide.

Copilot · 2026-01-08T10:16:23Z

src/main/java/com/xzavier0722/mc/plugin/slimefun4/storage/util/InvSnapshot.java

+    public InvSnapshot(List<Pair<ItemStack, Integer>> snapshot) {
+        this.snapshot = List.copyOf(snapshot);


The snapshot is immutable by using List.copyOf(), which is good for thread safety. However, the ItemStack objects within the list are still mutable. If ItemStacks are modified elsewhere, the snapshot integrity could be compromised. Consider creating defensive copies of ItemStacks within the snapshot.

Copilot · 2026-01-08T10:16:23Z

src/main/java/io/github/thebusybiscuit/slimefun4/implementation/listeners/BackpackListener.java

+                            if (backpacks.containsValue(bp.getUniqueId())
+                                    || !bp.getInventory().getViewers().isEmpty()) {
+                                Slimefun.getLocalization().sendMessage(p, "backpack.already-open", true);
+                                return;
+                            }
+                            SoundEffect.BACKPACK_OPEN_SOUND.playAt(p.getLocation(), SoundCategory.PLAYERS);
+                            bp.open(p);
+                            backpacks.put(p.getUniqueId(), bp.getUniqueId());
+                            backpackInstances.put(p.getUniqueId(), backpackItem);


There's a race condition between checking if the backpack is already open (line 264) and actually opening it (line 270-271). Another thread could open the same backpack between these operations. The check uses containsValue() which searches through all values, and then later uses put() which could overwrite. Consider using a more atomic operation or additional synchronization to prevent multiple viewers from opening the same backpack simultaneously.

Suggested change

if (backpacks.containsValue(bp.getUniqueId())

|| !bp.getInventory().getViewers().isEmpty()) {

Slimefun.getLocalization().sendMessage(p, "backpack.already-open", true);

return;

}

SoundEffect.BACKPACK_OPEN_SOUND.playAt(p.getLocation(), SoundCategory.PLAYERS);

bp.open(p);

backpacks.put(p.getUniqueId(), bp.getUniqueId());

backpackInstances.put(p.getUniqueId(), backpackItem);

synchronized (backpacks) {

if (backpacks.containsValue(bp.getUniqueId())

|| !bp.getInventory().getViewers().isEmpty()) {

Slimefun.getLocalization().sendMessage(p, "backpack.already-open", true);

return;

}

SoundEffect.BACKPACK_OPEN_SOUND.playAt(p.getLocation(), SoundCategory.PLAYERS);

bp.open(p);

backpacks.put(p.getUniqueId(), bp.getUniqueId());

backpackInstances.put(p.getUniqueId(), backpackItem);

}

Copilot · 2026-01-08T10:16:23Z

src/main/java/io/github/thebusybiscuit/slimefun4/implementation/listeners/BackpackListener.java

+        if (!openingPlayers.isEmpty() && openingPlayers.contains(e.getPlayer().getUniqueId())) {
+            e.setCancelled(true);
+            return;


The openingPlayers set is checked using isEmpty() before contains(). This is an unnecessary optimization that adds complexity. The contains() operation on a HashSet is O(1) regardless of whether isEmpty() is checked first. Consider removing the isEmpty() checks throughout this file to simplify the code.

Copilot · 2026-01-08T10:16:23Z

src/main/java/io/github/thebusybiscuit/slimefun4/implementation/listeners/BackpackListener.java

+                if (!Objects.equals(itemCopy, item)) {
+                    Slimefun.getLocalization().sendMessage(player, "backpack.not-original-item", true);
                    return;


The item is compared using equals() with the itemCopy that was cloned earlier. However, ItemStack.equals() compares object identity by default in some implementations, not deep equality. Consider using a more reliable comparison method such as ItemStack.isSimilar() or comparing the serialized data to detect if the item has been modified.

Copilot · 2026-01-08T10:16:24Z

src/main/java/io/github/thebusybiscuit/slimefun4/implementation/listeners/BackpackListener.java

+        openingPlayers.add(p.getUniqueId());
+        PlayerBackpack.getAsync(item)
+                .thenAcceptAsync(
+                        bp -> {
+                            openingPlayers.remove(p.getUniqueId());
+                            // Check if the backpack item contains invalid backpack data
+                            if (bp == null || bp.isInvalid()) {
+                                return;
+                            }
+                            // Check if someone else is currently viewing this backpack
+                            if (backpacks.containsValue(bp.getUniqueId())
+                                    || !bp.getInventory().getViewers().isEmpty()) {
+                                Slimefun.getLocalization().sendMessage(p, "backpack.already-open", true);
+                                return;
+                            }
+                            SoundEffect.BACKPACK_OPEN_SOUND.playAt(p.getLocation(), SoundCategory.PLAYERS);
+                            bp.open(p);
+                            backpacks.put(p.getUniqueId(), bp.getUniqueId());
+                            backpackInstances.put(p.getUniqueId(), backpackItem);
+                        },
+                        ThreadUtils.getMainThreadExecutor());


The player is removed from openingPlayers inside the async callback, but if an exception occurs during backpack loading or the callback is never executed, the player will remain in the openingPlayers set indefinitely. This will block the player from interacting with items, entities, or opening other backpacks. Consider using a try-finally block or adding timeout handling to ensure cleanup.

m1919810 added 3 commits January 6, 2026 16:29

fix: backpack fixes

c1a4381

fix: backpack fixes

8551154

fix: backpack fixes

38df2c5

StarWishsama requested a review from Copilot January 7, 2026 06:45

Copilot started reviewing on behalf of StarWishsama January 7, 2026 06:45 View session

Copilot AI reviewed Jan 7, 2026

View reviewed changes

fix: backpack fixes

34f799a

StarWishsama requested a review from Copilot January 7, 2026 08:12

Copilot started reviewing on behalf of StarWishsama January 7, 2026 08:12 View session

Copilot AI reviewed Jan 7, 2026

View reviewed changes

StarWishsama requested changes Jan 7, 2026

View reviewed changes

src/main/java/com/xzavier0722/mc/plugin/slimefun4/storage/callback/IAsyncReadCallback.java Outdated Show resolved Hide resolved

StarWishsama and others added 2 commits January 7, 2026 16:18

chore: fix typo

a6c6371

Co-authored-by: Copilot <[email protected]>

chore: revert deprecate IAsyncReadCallback

26bc332

StarWishsama requested changes Jan 7, 2026

View reviewed changes

m1919810 added 2 commits January 8, 2026 18:06

fix: backpack fixes

06fabab

Merge branch 'fix/backpack_issues' of github-m1919810:m1919810/Slimef…

f7a9535

…un-vModified into fix/backpack_issues

StarWishsama requested a review from Copilot January 8, 2026 10:12

Copilot started reviewing on behalf of StarWishsama January 8, 2026 10:12 View session

Copilot AI reviewed Jan 8, 2026

View reviewed changes

StarWishsama approved these changes Jan 8, 2026

View reviewed changes

StarWishsama merged commit 8f6affb into SlimefunGuguProject:dev Jan 8, 2026
17 checks passed

	// } else {
	// Slimefun.getLocalization().sendMessage(p, "backpack.already-open", true);
	// }

	if (!Objects.equals(itemCopy, item)) {
	if (!itemCopy.equals(item)) {

-@Getter
+@Getter
+/**
+ * Immutable snapshot of an inventory's contents.
+ * <p>
+ * An {@code InvSnapshot} captures the state of an {@link Inventory} (or a raw
+ * {@link ItemStack} array) at a specific point in time. The underlying list of
+ * item/slot pairs is defensively copied on construction and cannot be modified
+ * afterwards, ensuring that the snapshot remains stable even if the original
+ * inventory or array is later changed.
+ * </p>
+ * <p>
+ * This class is typically used to detect which slots have changed between two
+ * inventory states. A common pattern is:
+ * </p>
+ * <pre>{@code
+ * InvSnapshot before = new InvSnapshot(inventory);
+ * // ... mutate the inventory ...
+ * Set<Integer> changedSlots = before.getChangedSlots(inventory);
+ * }</pre>
+ * <p>
+ * Use {@link #getChangedSlots(ItemStack[])} or {@link #getChangedSlots(Inventory)}
+ * to compare the stored snapshot against a current inventory view and obtain the
+ * set of slot indices whose contents differ.
+ * </p>
+ */

	* This interface is deprecated and related methods will be replaced by methods using {@link java.util.concurrent.CompletableFuture} in the future, <- this work is a completable future
	* This interface is deprecated and related methods will be replaced by methods using {@link java.util.concurrent.CompletableFuture} in the future; this work aims to replace them with {@link java.util.concurrent.CompletableFuture}.

	// Check if the player change the amount of item
	// Check if the player changes the amount of the item

	Slimefun.getLocalization().sendMessage(p, "backpack.no-stack", true);
	Slimefun.getLocalization().sendMessage(player, "backpack.no-stack", true);

- * This interface is deprecated and related methods will be replaced by methods using {@link java.util.concurrent.CompletableFuture} in the future, <- this work is a completable future
- * related methods will be kept and mark deprecated
- * you can run your original {@link IAsyncReadCallback} callbacks using {@link java.util.concurrent.CompletableFuture#thenAcceptAsync(Consumer, java.util.concurrent.Executor)}, don't forget to check whether the accepted value is null or not
- * If you want to invoke callback not on the main thread, use
- * {@link ADataController#getCallbackExecutor}
- * If you want to invoke callback on the main thread , see
- * {@link ThreadUtils#getMainThreadExecutor()} or {@link ThreadUtils#getMainDelayedExecutor()}
- * @param <T>
+ * This interface is deprecated. Related methods are being replaced by methods using {@link java.util.concurrent.CompletableFuture}.
+ * Existing related methods will be kept and marked as deprecated for backward compatibility.
+ * You can run your original {@link IAsyncReadCallback} callbacks using
+ * {@link java.util.concurrent.CompletableFuture#thenAcceptAsync(Consumer, java.util.concurrent.Executor)}; don't forget to check whether the accepted value is null or not.
+ * If you want to invoke the callback not on the main thread, use
+ * {@link ADataController#getCallbackExecutor}.
+ * If you want to invoke the callback on the main thread, see
+ * {@link ThreadUtils#getMainThreadExecutor()} or {@link ThreadUtils#getMainDelayedExecutor()}.
+ * @param <T> the type of the result

		public InvSnapshot(List<Pair<ItemStack, Integer>> snapshot) {
		this.snapshot = List.copyOf(snapshot);

Uh oh!

Fix: backpack issues #1183

Fix: backpack issues #1183

Uh oh!

Conversation

m1919810 commented Jan 7, 2026

简介

相关的 Issues (没有可不填)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

StarWishsama Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

StarWishsama Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

StarWishsama Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!