Folia support#38
Conversation
# Conflicts: # build.gradle # src/main/java/tf/tuff/TuffX.java
|
Warning Review limit reached
More reviews will be available in 43 minutes and 41 seconds. Learn how PR review limits work. To continue reviewing without waiting, enable usage-based billing in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds FoliaLib 0.4.4 as a shaded dependency and migrates every Bukkit scheduler call site across the plugin to Folia's thread-aware API ( ChangesFolia Scheduler Migration & ViaBlockIds Improvements
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/tf/tuff/netty/ChunkHandler.java (1)
217-223:⚠️ Potential issue | 🟠 MajorPin Y0 cache work to the same world captured at schedule time.
Line 223 calls
y0.cacheChunkWithCallback(player, cx, cz, ...), but that method resolvesplayer.getWorld()internally (line 575 of Y0Plugin.java). If the player changes worlds before this task runs, the task executes on one world/region (line 217) and touches another, which is unsafe under Folia thread ownership.The pattern in
requestViaCache(line 200) correctly captures the world upfront before passing it to the callback. Apply the same approach torequestY0Cache.Proposed fix
private void requestY0Cache(int cx, int cz, long key) { - Location chunkLoc = new Location(player.getWorld(), cx << 4, 0, cz << 4); + World scheduledWorld = player.getWorld(); + Location chunkLoc = new Location(scheduledWorld, cx << 4, 0, cz << 4); y0.getTuffX().foliaLib.getScheduler().runAtLocation(chunkLoc, t -> { if (!player.isOnline()) { release(key); return; } + if (player.getWorld() != scheduledWorld) { + QueuedPacket q = queue.get(key); + if (q != null) { + q.y0Ready = true; + tryRelease(key); + } + return; + } y0.cacheChunkWithCallback(player, cx, cz, data -> { QueuedPacket q = queue.get(key); if (q != null) { q.y0Data = data; q.y0Ready = 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/netty/ChunkHandler.java` around lines 217 - 223, The requestY0Cache method in ChunkHandler.java schedules work at a specific world location (line 217) but then calls y0.cacheChunkWithCallback which internally resolves the player's world at callback execution time. If the player changes worlds between scheduling and execution, this causes unsafe cross-world access. Capture player.getWorld() at the time of scheduling (before the runAtLocation call) and pass this captured world value to cacheChunkWithCallback instead of letting it resolve the world internally, following the same pattern used in requestViaCache at line 200.
🤖 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/ServerRegistry.java`:
- Around line 21-22: The `running` field in the ServerRegistry class is a plain
boolean that is read from async/WebSocket callbacks and written in the
`disconnect()` method, creating a visibility race condition where stale reads
can allow `doConnect()` retries to be scheduled after shutdown. Make the
`running` field thread-safe by declaring it as `volatile` to ensure visibility
of writes across threads, or alternatively use `AtomicBoolean` if you need
stronger guarantees. This ensures that when `disconnect()` sets `running` to
false, all subsequent async callbacks will see the updated value and skip
scheduling reconnection attempts.
---
Outside diff comments:
In `@src/main/java/tf/tuff/netty/ChunkHandler.java`:
- Around line 217-223: The requestY0Cache method in ChunkHandler.java schedules
work at a specific world location (line 217) but then calls
y0.cacheChunkWithCallback which internally resolves the player's world at
callback execution time. If the player changes worlds between scheduling and
execution, this causes unsafe cross-world access. Capture player.getWorld() at
the time of scheduling (before the runAtLocation call) and pass this captured
world value to cacheChunkWithCallback instead of letting it resolve the world
internally, following the same pattern used in requestViaCache at line 200.
🪄 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: 42afe9d1-6182-4aba-80f1-8900d7124c67
📒 Files selected for processing (15)
build.gradlesrc/main/java/tf/tuff/ServerRegistry.javasrc/main/java/tf/tuff/TuffX.javasrc/main/java/tf/tuff/netty/ChunkHandler.javasrc/main/java/tf/tuff/tuffactions/creative/CreativeMenu.javasrc/main/java/tf/tuff/tuffactions/swimming/Swimming.javasrc/main/java/tf/tuff/viablocks/CustomBlockListener.javasrc/main/java/tf/tuff/viablocks/PaletteManager.javasrc/main/java/tf/tuff/viablocks/ViaBlocksPlugin.javasrc/main/java/tf/tuff/viaentities/EntityInjector.javasrc/main/java/tf/tuff/y0/ChunkPacketListener.javasrc/main/java/tf/tuff/y0/ViaBlockIds.javasrc/main/java/tf/tuff/y0/Y0Plugin.javasrc/main/resources/mapping-26.1.2.jsonsrc/main/resources/plugin.yml
1 participant
First ever PR on GitHub, please be lenient if I majorly screwed something up. Adds Folia support, and also 26.1.2 mappings (and support for it), so ViaBlocks plays nice.
Uses FoliaLib since the project still aims for compat with spigot and versions as old as 1.18... If this is a problem I can figure something else out.
Tested on a live server, and works as intended. CI started failing after pulling upstream to latest though.
Summary by CodeRabbit
New Features
Chores