Fix runtime packet handling and swimming state#37
Conversation
📝 WalkthroughWalkthroughThis PR fixes thread safety issues in async block-change handling by moving Via data resolution to the main thread, refactors swimming state management via periodic scheduler tasks, adds resource and state guards throughout ViaBlocks and Y0 plugins, and establishes JUnit 5 testing infrastructure. ChangesCore Thread Safety and State Management
🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/main/java/tf/tuff/netty/ChunkHandler.java (1)
160-160: 📐 Maintainability & Code Quality | 💤 Low valueUnnecessary decode call.
decodeSectionCoordX(chunkSectionPos)is called but its result is discarded. The comment says it "validates bit layout," but no validation exception is thrown, and the coordinates are decoded again insidedecodeMultiBlockChangePosition. This call can be removed.Suggested fix
- decodeSectionCoordX(chunkSectionPos); // validates bit layout alongside the packed entries below buf.readBoolean();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/tf/tuff/netty/ChunkHandler.java` at line 160, Remove the redundant call to decodeSectionCoordX(chunkSectionPos) — it returns a value that is discarded and the coordinates are already decoded inside decodeMultiBlockChangePosition; delete the standalone call and any comment that implies validation occurs there, leaving decodeMultiBlockChangePosition to perform the decoding/validation for chunkSectionPos.src/main/java/tf/tuff/TuffX.java (1)
93-105: 📐 Maintainability & Code Quality | ⚡ Quick winConsider also unregistering outgoing plugin channels for completeness.
The incoming channel cleanup is correct, but sub-plugins like ViaEntitiesPlugin register outgoing channels with
TuffXas the plugin owner. For defensive cleanup, consider also callinggetServer().getMessenger().unregisterOutgoingPluginChannel(this);to ensure all registered channels are cleaned up during shutdown.♻️ Proposed enhancement
PacketEvents.getAPI().terminate(); getServer().getMessenger().unregisterIncomingPluginChannel(this); + getServer().getMessenger().unregisterOutgoingPluginChannel(this); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/tf/tuff/TuffX.java` around lines 93 - 105, The onDisable method currently unregisters incoming plugin channels but misses outgoing channels; update onDisable in TuffX.java (method onDisable) to also call getServer().getMessenger().unregisterOutgoingPluginChannel(this) (in addition to the existing unregisterIncomingPluginChannel(this)) so any outgoing channels registered by sub-plugins (e.g., ViaEntitiesPlugin) are cleaned up; ensure this call is placed alongside the existing messenger cleanup and executed before returning from onDisable.src/main/java/tf/tuff/tuffactions/swimming/Swimming.java (2)
127-142: 📐 Maintainability & Code Quality | 💤 Low valueUse explicit Iterator for cleaner removal during iteration.
While
ConcurrentHashMap.newKeySet()won't throwConcurrentModificationException, removing via the set reference (rather thaniterator.remove()) during iteration can lead to skipped elements in edge cases. Using an explicit Iterator provides more predictable behavior.♻️ Suggested refactor using Iterator
private void maintainSwimmingState() { - for (UUID playerId : swimmingPlayers) { + var iterator = swimmingPlayers.iterator(); + while (iterator.hasNext()) { + UUID playerId = iterator.next(); Player player = Bukkit.getPlayer(playerId); if (player == null || !player.isOnline()) { - swimmingPlayers.remove(playerId); + iterator.remove(); continue; } if (!player.isInWater()) { - swimmingPlayers.remove(playerId); + iterator.remove(); applySwimmingState(player, false); broadcastSwimState(player, false); continue; } applySwimmingState(player, true); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/tf/tuff/tuffactions/swimming/Swimming.java` around lines 127 - 142, The loop in maintainSwimmingState removes from swimmingPlayers directly while iterating, which can skip elements; change it to use an explicit Iterator over swimmingPlayers (Iterator<UUID> it = swimmingPlayers.iterator()) and call it.remove() when you need to remove the current UUID; keep the same logic calling Bukkit.getPlayer(playerId), applySwimmingState(player, false/true) and broadcastSwimState(player, false) but ensure removals use iterator.remove() so iteration is predictable.
79-90: 🎯 Functional Correctness | 💤 Low valueConsider cancelling the event to prevent state flicker.
The current approach lets the event proceed (setting swimming to false), then schedules a task to re-apply swimming. This creates a brief one-tick window where the player's swimming state is false. Cancelling the event directly would be more responsive and prevent the intermediate state.
♻️ Alternative approach using event cancellation
public void handleToggleSwim(EntityToggleSwimEvent event) { if (!isEnabled()) return; if (!(event.getEntity() instanceof Player)) return; Player player = (Player) event.getEntity(); if (!event.isSwimming() && swimmingPlayers.contains(player.getUniqueId())) { - plugin.getServer().getScheduler().runTask(plugin, () -> { - if (swimmingPlayers.contains(player.getUniqueId()) && player.isOnline()) { - applySwimmingState(player, true); - } - }); + event.setCancelled(true); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/tf/tuff/tuffactions/swimming/Swimming.java` around lines 79 - 90, The handleToggleSwim method presently lets EntityToggleSwimEvent run then re-applies swimming on the next tick causing a flicker; change the logic so when the event is coming from a Player and you detect that swimming should remain true (i.e., !event.isSwimming() && swimmingPlayers.contains(player.getUniqueId())), call event.setCancelled(true) on that EntityToggleSwimEvent to prevent the state flip and then ensure you call applySwimmingState(player, true) (immediately or via the existing scheduler if needed) so the player stays in the expected swimming state without the one-tick false window; update references in handleToggleSwim and keep applySwimmingState usage consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/tf/tuff/netty/ChunkHandler.java`:
- Around line 172-202: The leak is caused by double-retaining the ByteBuf:
resolveViaDataOnMainThread retains buf and then writeWithViaOnly retains it
again for the composite; remove the extra retain in writeWithViaOnly so the
composite takes the caller's ownership, and update resolveViaDataOnMainThread so
you do not release the retained buffer in the finally block when you call
writeWithViaOnly (ensure only the code path that does currentCtx.write(...)
releases the retained buffer). Touch the methods resolveViaDataOnMainThread and
writeWithViaOnly to transfer ownership consistently (caller retains once, callee
must not retain) and ensure all other paths still release appropriately.
In `@src/main/java/tf/tuff/tuffactions/swimming/Swimming.java`:
- Around line 40-46: The enable method's null check on swimStateTask can miss
cases where the Task object exists but the scheduled task was cancelled; update
the scheduling guard in enable(...) to schedule a new task when swimStateTask is
null OR swimStateTask.isCancelled() is true, using the same call to
plugin.getServer().getScheduler().runTaskTimer(...) to assign swimStateTask;
reference swimStateTask, enable, maintainSwimmingState and the scheduler
runTaskTimer call when making the change.
In `@src/main/java/tf/tuff/viablocks/ViaBlocksPlugin.java`:
- Around line 82-83: Reload currently clears the viaBlocksEnabledPlayers set in
onTuffXReload which causes online players to lose ViaBlocks-enabled state and
stop receiving chunk/block packets until they re-handshake; instead preserve or
rebuild enabled-player state across reloads by iterating online players after
reload and repopulating viaBlocksEnabledPlayers (or trigger a re-handshake) for
those where CustomBlockListener.hasViaBlocksPlayersInWorld(...) /
plugin.isPlayerEnabled(...) previously returned true; update onTuffXReload to
either restore the set from a persisted snapshot or call the same logic used in
handlePacket -> setPlayerEnabled(player, true) for each online player so
CustomBlockListener.handleChunkLoad continues to prepare caches without
requiring clients to resend viablocks:handshake.
---
Nitpick comments:
In `@src/main/java/tf/tuff/netty/ChunkHandler.java`:
- Line 160: Remove the redundant call to decodeSectionCoordX(chunkSectionPos) —
it returns a value that is discarded and the coordinates are already decoded
inside decodeMultiBlockChangePosition; delete the standalone call and any
comment that implies validation occurs there, leaving
decodeMultiBlockChangePosition to perform the decoding/validation for
chunkSectionPos.
In `@src/main/java/tf/tuff/tuffactions/swimming/Swimming.java`:
- Around line 127-142: The loop in maintainSwimmingState removes from
swimmingPlayers directly while iterating, which can skip elements; change it to
use an explicit Iterator over swimmingPlayers (Iterator<UUID> it =
swimmingPlayers.iterator()) and call it.remove() when you need to remove the
current UUID; keep the same logic calling Bukkit.getPlayer(playerId),
applySwimmingState(player, false/true) and broadcastSwimState(player, false) but
ensure removals use iterator.remove() so iteration is predictable.
- Around line 79-90: The handleToggleSwim method presently lets
EntityToggleSwimEvent run then re-applies swimming on the next tick causing a
flicker; change the logic so when the event is coming from a Player and you
detect that swimming should remain true (i.e., !event.isSwimming() &&
swimmingPlayers.contains(player.getUniqueId())), call event.setCancelled(true)
on that EntityToggleSwimEvent to prevent the state flip and then ensure you call
applySwimmingState(player, true) (immediately or via the existing scheduler if
needed) so the player stays in the expected swimming state without the one-tick
false window; update references in handleToggleSwim and keep applySwimmingState
usage consistent.
In `@src/main/java/tf/tuff/TuffX.java`:
- Around line 93-105: The onDisable method currently unregisters incoming plugin
channels but misses outgoing channels; update onDisable in TuffX.java (method
onDisable) to also call
getServer().getMessenger().unregisterOutgoingPluginChannel(this) (in addition to
the existing unregisterIncomingPluginChannel(this)) so any outgoing channels
registered by sub-plugins (e.g., ViaEntitiesPlugin) are cleaned up; ensure this
call is placed alongside the existing messenger cleanup and executed before
returning from onDisable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 80cf6668-62b1-44ac-b6d7-b087a2f792bc
⛔ Files ignored due to path filters (1)
builds/TuffXPlus-1.0.1-beta.jaris excluded by!**/*.jar
📒 Files selected for processing (8)
build.gradlesrc/main/java/tf/tuff/TuffX.javasrc/main/java/tf/tuff/netty/ChunkHandler.javasrc/main/java/tf/tuff/tuffactions/swimming/Swimming.javasrc/main/java/tf/tuff/viablocks/CustomBlockListener.javasrc/main/java/tf/tuff/viablocks/ViaBlocksPlugin.javasrc/main/java/tf/tuff/y0/Y0Plugin.javasrc/test/java/tf/tuff/netty/ChunkHandlerTest.java
| private void resolveViaDataOnMainThread(ChannelHandlerContext ctx, ByteBuf buf, ChannelPromise promise, | ||
| java.util.concurrent.Callable<byte[]> supplier) { | ||
| ByteBuf retained = buf.retain(); | ||
| Bukkit.getScheduler().runTask(viaBlocks.plugin.plugin, () -> { | ||
| byte[] data = null; | ||
| try { | ||
| if (player.isOnline() && isViaActive()) { | ||
| data = supplier.call(); | ||
| } | ||
| } catch (Exception ignored) { | ||
| } | ||
| final byte[] resolvedData = data; | ||
|
|
||
| ChannelHandlerContext currentCtx = this.ctx != null ? this.ctx : ctx; | ||
| currentCtx.channel().eventLoop().execute(() -> { | ||
| try { | ||
| retained.resetReaderIndex(); | ||
| if (resolvedData != null && resolvedData.length > 0) { | ||
| writeWithViaOnly(currentCtx, retained, promise, resolvedData); | ||
| } else { | ||
| currentCtx.write(retained, promise); | ||
| } | ||
| } catch (Exception ignored) { | ||
| } finally { | ||
| if (retained.refCnt() > 0) { | ||
| retained.release(); | ||
| } | ||
| } | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Memory leak in writeWithViaOnly path.
When resolveViaDataOnMainThread takes the writeWithViaOnly path, there's one unreleased buffer reference:
bufstarts withrefCnt=1- Line 174:
buf.retain()→refCnt=2 - Line 314 (
writeWithViaOnly):buf.retain()→refCnt=3, composite takes ownership of this reference - Line 197:
retained.release()→refCnt=2 - After
ctx.write(composite)completes, composite releases its component →refCnt=1
The original reference is never released, causing a memory leak.
In writeWithViaOnly, you're retaining the buffer to add it to the composite, but the caller (resolveViaDataOnMainThread) also holds a retained reference. You should not retain again in writeWithViaOnly since the caller is passing ownership of its retained reference.
Proposed fix
private void writeWithViaOnly(ChannelHandlerContext ctx, ByteBuf buf, ChannelPromise promise,
byte[] data) throws Exception {
ByteBuf tail = ctx.alloc().buffer();
tail.writeBytes(data);
CompositeByteBuf composite = ctx.alloc().compositeBuffer();
- composite.addComponents(true, buf.retain(), tail);
+ composite.addComponents(true, buf, tail);
ctx.write(composite, promise);
}And adjust resolveViaDataOnMainThread to not release when writing via writeWithViaOnly (since composite takes ownership), or remove the finally release entirely and let the write operations handle it:
try {
retained.resetReaderIndex();
if (resolvedData != null && resolvedData.length > 0) {
writeWithViaOnly(currentCtx, retained, promise, resolvedData);
} else {
currentCtx.write(retained, promise);
}
} catch (Exception ignored) {
- } finally {
- if (retained.refCnt() > 0) {
- retained.release();
- }
+ } catch (Exception e) {
+ if (retained.refCnt() > 0) {
+ retained.release();
+ }
}Also applies to: 308-317
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/tf/tuff/netty/ChunkHandler.java` around lines 172 - 202, The
leak is caused by double-retaining the ByteBuf: resolveViaDataOnMainThread
retains buf and then writeWithViaOnly retains it again for the composite; remove
the extra retain in writeWithViaOnly so the composite takes the caller's
ownership, and update resolveViaDataOnMainThread so you do not release the
retained buffer in the finally block when you call writeWithViaOnly (ensure only
the code path that does currentCtx.write(...) releases the retained buffer).
Touch the methods resolveViaDataOnMainThread and writeWithViaOnly to transfer
ownership consistently (caller retains once, callee must not retain) and ensure
all other paths still release appropriately.
| @Override | ||
| protected void enable(boolean wasEnabled) { | ||
| if (swimStateTask == null) { | ||
| swimStateTask = plugin.getServer().getScheduler().runTaskTimer(plugin, this::maintainSwimmingState, 1L, 1L); | ||
| } | ||
| super.enable(wasEnabled); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Consider checking isCancelled() for robustness.
The null check prevents duplicate tasks in normal scenarios, but if the task was cancelled externally (e.g., scheduler cleared), swimStateTask would remain non-null while the task isn't running. Using swimStateTask.isCancelled() provides a more robust check.
🛡️ Suggested fix
`@Override`
protected void enable(boolean wasEnabled) {
- if (swimStateTask == null) {
+ if (swimStateTask == null || swimStateTask.isCancelled()) {
swimStateTask = plugin.getServer().getScheduler().runTaskTimer(plugin, this::maintainSwimmingState, 1L, 1L);
}
super.enable(wasEnabled);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/tf/tuff/tuffactions/swimming/Swimming.java` around lines 40 -
46, The enable method's null check on swimStateTask can miss cases where the
Task object exists but the scheduled task was cancelled; update the scheduling
guard in enable(...) to schedule a new task when swimStateTask is null OR
swimStateTask.isCancelled() is true, using the same call to
plugin.getServer().getScheduler().runTaskTimer(...) to assign swimStateTask;
reference swimStateTask, enable, maintainSwimmingState and the scheduler
runTaskTimer call when making the change.
| viaBlocksEnabledPlayers.clear(); | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for player state preservation patterns during reload
# Check for similar state management during reload/enable cycles
rg -nC3 'onTuffXReload|onReload' --type=java
# Look for player state preservation or migration patterns
rg -nC3 'viaBlocksEnabledPlayers' --type=javaRepository: TuffNetwork/TuffXPlus
Length of output: 7845
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect reload/disable/handshake-related code paths for viaBlocksEnabledPlayers
sed -n '1,220p' src/main/java/tf/tuff/viablocks/ViaBlocksPlugin.java | nl -ba | sed -n '40,120p'
sed -n '220,320p' src/main/java/tf/tuff/viablocks/ViaBlocksPlugin.java | nl -ba
# Inspect the cache/guard logic in CustomBlockListener
sed -n '600,760p' src/main/java/tf/tuff/viablocks/CustomBlockListener.java | nl -ba
# Find where handshake/data channel sets player enabled
rg -n "SERVERBOUND_CHANNEL|CLIENTBOUND_CHANNEL|handshake|setPlayerEnabled|viaBlocksEnabledPlayers\.add|viaBlocksEnabledPlayers\.remove" src/main/java/tf/tuff/viablocks -S
# Look for other player-state migration/preservation during reload/enable cycles (broader than onTuffXReload)
rg -nC3 "(onTuffXReload|onEnable|onDisable|reload)" src/main/java/tf/tuff -S --type=javaRepository: TuffNetwork/TuffXPlus
Length of output: 108
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== ViaBlocksPlugin.java: onTuffXReload / disable around viaBlocksEnabledPlayers.clear ==="
sed -n '1,220p' src/main/java/tf/tuff/viablocks/ViaBlocksPlugin.java | cat -n | sed -n '40,120p'
sed -n '1,220p' src/main/java/tf/tuff/viablocks/ViaBlocksPlugin.java | cat -n | sed -n '140,210p'
sed -n '1,220p' src/main/java/tf/tuff/viablocks/ViaBlocksPlugin.java | cat -n | sed -n '210,280p'
echo
echo "=== CustomBlockListener.java: hasViaBlocksPlayersInWorld and related guards ==="
sed -n '560,740p' src/main/java/tf/tuff/viablocks/CustomBlockListener.java | cat -n | sed -n '1,220p'
echo
echo "=== Handshake / setPlayerEnabled usage to see how online players get re-added ==="
rg -n "setPlayerEnabled\\(|viaBlocksEnabledPlayers\\.add\\(|viaBlocksEnabledPlayers\\.remove\\(|SERVERBOUND_CHANNEL|CLIENTBOUND_CHANNEL|handshake" src/main/java/tf/tuff/viablocks -S --type=java
echo
echo "=== Any other reload/enable patterns elsewhere (broader signal) ==="
rg -nC3 "(onTuffXReload|onEnable|onDisable|reload\\b)" src/main/java/tf/tuff -S --type=javaRepository: TuffNetwork/TuffXPlus
Length of output: 19721
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== ViaBlocksPlugin.java: incoming handshake handling around CLIENTBOUND/SERVERBOUND registration ==="
sed -n '100,190p' src/main/java/tf/tuff/viablocks/ViaBlocksPlugin.java | cat -n
echo
echo "=== Call sites: hasViaBlocksPlayersInWorld(...) to see what it guards ==="
rg -n "hasViaBlocksPlayersInWorld\\(" src/main/java/tf/tuff/viablocks/CustomBlockListener.java src/main/java/tf/tuff/viablocks/*.java
echo
echo "=== CustomBlockListener: chunk caching logic around those guards ==="
# Print a wider window where caching likely happens
sed -n '300,560p' src/main/java/tf/tuff/viablocks/CustomBlockListener.java | cat -nRepository: TuffNetwork/TuffXPlus
Length of output: 16460
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== CustomBlockListener.java: context around hasViaBlocksPlayersInWorld call site ==="
sed -n '150,220p' src/main/java/tf/tuff/viablocks/CustomBlockListener.java | cat -nRepository: TuffNetwork/TuffXPlus
Length of output: 3584
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "viaBlocksEnabledPlayers\\.(add|remove|clear)" src/main/java/tf/tuff/viablocks -SRepository: TuffNetwork/TuffXPlus
Length of output: 621
Consider UX impact of clearing viaBlocksEnabledPlayers on reload
ViaBlocksPlugin.onTuffXReload() clears viaBlocksEnabledPlayers (and the CustomBlockListener cache). That set is then required to drive custom rendering: CustomBlockListener.handleChunkLoad only prepares chunk cache when hasViaBlocksPlayersInWorld(...) finds an online player in the set, and most update paths gate on plugin.isPlayerEnabled(player). As a result, online players won’t receive ViaBlocks chunk/block packets after a reload until they send the viablocks:handshake again (handled by handlePacket() → setPlayerEnabled(player, true)). If this full state reset is intended, it should be explicitly accounted for; otherwise, preserve/rebuild the enabled-player state across reloads (or trigger re-handshake for connected clients).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/tf/tuff/viablocks/ViaBlocksPlugin.java` around lines 82 - 83,
Reload currently clears the viaBlocksEnabledPlayers set in onTuffXReload which
causes online players to lose ViaBlocks-enabled state and stop receiving
chunk/block packets until they re-handshake; instead preserve or rebuild
enabled-player state across reloads by iterating online players after reload and
repopulating viaBlocksEnabledPlayers (or trigger a re-handshake) for those where
CustomBlockListener.hasViaBlocksPlayersInWorld(...) /
plugin.isPlayerEnabled(...) previously returned true; update onTuffXReload to
either restore the set from a persisted snapshot or call the same logic used in
handlePacket -> setPlayerEnabled(player, true) for each online player so
CustomBlockListener.handleChunkLoad continues to prepare caches without
requiring clients to resend viablocks:handshake.
This fixes the async block read path, the broken section block update decoding below Y0, the unreliable Viablocks chunk send path, and the swim state regression. It also cuts cache churn when the relevant features are off or no Tuff players are active.
Closes #21
Closes #14
Closes #8
Closes #5
Refs #17
Refs #30
Summary by CodeRabbit
Bug Fixes
Tests