Workaround for client-sided removal of all items from the crafting grid on inventory close#6008
Conversation
…en closing the inventory"
…ntory-closing-all-items-gone
There was a problem hiding this comment.
Pull request overview
This PR implements a workaround for a Bedrock Edition client quirk where all items are removed from the crafting grid when closing the inventory, which causes issues with some Java Edition plugins. The solution introduces a delayed transaction mechanism that can detect and defer these crafting-to-inventory transfers until it's confirmed the inventory isn't being closed.
- Introduces
InterruptibleFutureutility class to manage delayed/cancellable scheduled tasks - Adds detection logic to identify crafting grid item removal patterns and delay their processing
- Integrates the interrupt mechanism into container close events to prevent false transactions
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
InterruptibleFuture.java |
New utility class for managing interruptible scheduled futures with cancel-and-run semantics |
BedrockContainerCloseTranslator.java |
Cancels pending inventory transactions when container is closed |
InventoryTranslator.java |
Implements detection and delayed processing of crafting grid item removal, fixes typo in comment |
GeyserSession.java |
Adds inventoryTransactionFuture field to track delayed inventory transactions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
|
|
||
| public void schedule(Runnable runnable, long delay, TimeUnit unit) { | ||
| runCurrentIfPresent(); | ||
| this.future = session.scheduleInEventLoop(this.runnable = runnable, delay, unit); |
There was a problem hiding this comment.
The assignment order in this chained expression could lead to a race condition. The expression session.scheduleInEventLoop(this.runnable = runnable, delay, unit) assigns to this.runnable after passing the runnable to the scheduler, but without proper synchronization, the scheduled task could potentially execute before the field assignment is visible. Consider separating the assignment: first assign this.runnable = runnable, then call this.future = session.scheduleInEventLoop(runnable, delay, unit).
| this.future = session.scheduleInEventLoop(this.runnable = runnable, delay, unit); | |
| this.runnable = runnable; | |
| this.future = session.scheduleInEventLoop(runnable, delay, unit); |
| public void runCurrentIfPresent() { | ||
| if (future != null && future.cancel(false) && runnable != null) { | ||
| runnable.run(); | ||
| } |
There was a problem hiding this comment.
The runCurrentIfPresent() method has a potential race condition. If the scheduled task starts executing between the cancel(false) call and the runnable.run() call, the runnable could be executed twice - once by the scheduler and once by this method. Consider using cancel(true) to interrupt the task or add synchronization to prevent concurrent execution.
| private @Nullable ScheduledFuture<?> future = null; | ||
| private @Nullable Runnable runnable = null; |
There was a problem hiding this comment.
The fields future and runnable are not synchronized, which can lead to visibility issues in a multi-threaded environment. Since this class is used with scheduled tasks that may execute on different threads, these fields should be either volatile or accessed within synchronized blocks to ensure proper memory visibility.
| private @Nullable ScheduledFuture<?> future = null; | |
| private @Nullable Runnable runnable = null; | |
| private volatile @Nullable ScheduledFuture<?> future = null; | |
| private volatile @Nullable Runnable runnable = null; |
|
|
||
| if (mustDelay) { | ||
| // We cannot clearly differentiate shift-clicking a single stack... | ||
| // So we try to guess and delay the processing (at least 10 ticks) until we either get a new inventory transaction, |
There was a problem hiding this comment.
The comment incorrectly states "at least 10 ticks" when the actual delay is 750 milliseconds (approximately 37.5 ticks at 20 ticks/second). Either update the comment to match the actual delay (e.g., "at least 750ms" or "approximately 15 ticks"), or adjust the delay value if 10 ticks was the intended duration (500ms).
| // So we try to guess and delay the processing (at least 10 ticks) until we either get a new inventory transaction, | |
| // So we try to guess and delay the processing (at least 750ms, approximately 15 ticks) until we either get a new inventory transaction, |
Would resolve #5258, fix #3932