Skip to content

fix: improve lock and sync when calling and trimming - #3543

Open
dordsor21 wants to merge 5 commits into
mainfrom
fiix/call-trim-deadlock
Open

fix: improve lock and sync when calling and trimming#3543
dordsor21 wants to merge 5 commits into
mainfrom
fiix/call-trim-deadlock

Conversation

@dordsor21

Copy link
Copy Markdown
Member

@dordsor21
dordsor21 requested a review from a team as a code owner May 24, 2026 13:21

@PierreSchwang PierreSchwang left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Untested, but looks reasonable so far

Comment on lines +156 to +160
fieldPendingBlockEntities = ChunkAccess.class.getDeclaredField(Refraction.pickName(
"pendingBlockEntities",
"i"
)
);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There shouldn't be any obfuscation anymore, right?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No idea, I was just going along with everything... If it's been removed we can clean up a bunch of this class I guess? (This field is both paper and spigot fwiw)

 - also fix error when editing newly generated chunks due to postprocessgen not having been completed
 - fixes #3490
@dordsor21
dordsor21 force-pushed the fiix/call-trim-deadlock branch from a6a08b7 to 45c46ae Compare July 25, 2026 15:31
@dordsor21
dordsor21 requested a review from a team July 26, 2026 11:12
@MattBDev

MattBDev commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Code review

Found 2 issues. Both are single-adapter divergences — a change applied correctly to 7 of the 8 per-version copies, missed in the 8th.

  1. Lock-order inversion in adapter-26.2 only. internalCall now enters synchronized (this) before taking sectionLock.writeLock(), but trim(true) in the same file takes sectionLock.writeLock() first and then synchronized (this). That is an AB-BA inversion between an edit thread and cache trimming — the exact interaction this PR is meant to fix. The base revision had no synchronized (this) here at all, so the PR introduces it.

try {
synchronized (this) {
sectionLock.writeLock().lock();
if (this.getChunk() != nmsChunk) {
this.levelChunk = nmsChunk;

Opposite order in trim(boolean aggressive) in the same file:

if (aggressive) {
sectionLock.writeLock().lock();
try {
synchronized (this) {
skyLight = new DataLayer[getSectionCount()];
blockLight = new DataLayer[getSectionCount()];

The other seven adapters lock in the order that matches their own trim(), e.g. adapter-26.1:

try {
sectionLock.writeLock().lock();
synchronized (this) {
if (this.getChunk() != nmsChunk) {
this.levelChunk = nmsChunk;

  1. adapter-1_21 is missing the boolean createCopy = this.createCopy; snapshot that the other seven adapters gained in this PR. copy is decided once from the field, but the deferred syncTasks lambda re-reads the live, non-volatile AbstractBukkitGetBlocks.createCopy when it later runs on the sync thread. If setCreateCopy flips the field in between, createCopy reads true while copy is still null, NPE-ing on copy.storeEntity(entity).

) throws Exception {
Map<BlockPos, CompoundTag> tilesToInit = PaperweightPlatformAdapter.clearPostProcessing(nmsChunk, false);
PaperweightGetBlocks_Copy copy = createCopy ? new PaperweightGetBlocks_Copy(nmsChunk) : null;
if (createCopy) {
if (copies.containsKey(copyKey)) {

Deferred read of the live field:

if (entityRemoves.contains(uuid)) {
if (createCopy) {
copy.storeEntity(entity);
}
removeEntity(entity);

Compare adapter-1_21_11, which has the snapshot:

Map<BlockPos, CompoundTag> tilesToInit = PaperweightPlatformAdapter.clearPostProcessing(nmsChunk, false);
boolean createCopy = this.createCopy;
PaperweightGetBlocks_Copy copy = createCopy ? new PaperweightGetBlocks_Copy(nmsChunk) : null;

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR focuses on improving concurrency/locking behavior during chunk “call” execution and trimming, and adds handling for editing chunks that are newly generated but have not completed post-processing (pending block entity initialization), addressing issue #3490.

Changes:

  • Adjust chunk GET call synchronization/visibility behavior in the shared Bukkit adapter base.
  • Add logic across multiple Paperweight adapters to temporarily manage pendingBlockEntities during edits when post-processing hasn’t completed.
  • Refactor sync-task construction and trim logic in multiple versioned adapters.

Reviewed changes

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

Show a summary per file
File Description
worldedit-bukkit/src/main/java/com/fastasyncworldedit/bukkit/adapter/AbstractBukkitGetBlocks.java Removes synchronized from call() (relying on callLock) and makes forceLoadSections volatile for cross-thread visibility.
worldedit-bukkit/adapters/adapter-26.2/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v26_2/PaperweightPlatformAdapter.java Adds reflective accessors to clear/restore pendingBlockEntities for post-processing handling.
worldedit-bukkit/adapters/adapter-26.2/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v26_2/PaperweightGetBlocks.java Uses new post-processing helpers; refactors sync task construction; adjusts trimming flow.
worldedit-bukkit/adapters/adapter-26.1/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v26_1/PaperweightPlatformAdapter.java Adds reflective accessors to clear/restore pendingBlockEntities for post-processing handling.
worldedit-bukkit/adapters/adapter-26.1/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v26_1/PaperweightGetBlocks.java Uses new post-processing helpers; refactors sync task construction; adjusts trimming flow.
worldedit-bukkit/adapters/adapter-1_21/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_21_R1/PaperweightPlatformAdapter.java Adds reflective accessors to clear/restore pendingBlockEntities for post-processing handling.
worldedit-bukkit/adapters/adapter-1_21/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_21_R1/PaperweightGetBlocks.java Uses new post-processing helpers; refactors sync task construction and trim logic.
worldedit-bukkit/adapters/adapter-1_21_9/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_21_9/PaperweightPlatformAdapter.java Adds reflective accessors to clear/restore pendingBlockEntities for post-processing handling.
worldedit-bukkit/adapters/adapter-1_21_9/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_21_9/PaperweightGetBlocks.java Uses new post-processing helpers; refactors sync task construction; adjusts trimming flow.
worldedit-bukkit/adapters/adapter-1_21_6/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_21_6/PaperweightPlatformAdapter.java Adds reflective accessors to clear/restore pendingBlockEntities for post-processing handling.
worldedit-bukkit/adapters/adapter-1_21_6/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_21_6/PaperweightGetBlocks.java Uses new post-processing helpers; refactors sync task construction; adjusts trimming flow.
worldedit-bukkit/adapters/adapter-1_21_5/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_21_5/PaperweightPlatformAdapter.java Adds reflective accessors to clear/restore pendingBlockEntities for post-processing handling.
worldedit-bukkit/adapters/adapter-1_21_5/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_21_5/PaperweightGetBlocks.java Uses new post-processing helpers; refactors sync task construction; adjusts trimming flow.
worldedit-bukkit/adapters/adapter-1_21_4/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_21_4/PaperweightPlatformAdapter.java Adds reflective accessors to clear/restore pendingBlockEntities for post-processing handling.
worldedit-bukkit/adapters/adapter-1_21_4/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_21_4/PaperweightGetBlocks.java Uses new post-processing helpers; refactors sync task construction; adjusts trimming flow.
worldedit-bukkit/adapters/adapter-1_21_11/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_21_11/PaperweightPlatformAdapter.java Adds reflective accessors to clear/restore pendingBlockEntities for post-processing handling.
worldedit-bukkit/adapters/adapter-1_21_11/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_21_11/PaperweightGetBlocks.java Uses new post-processing helpers; refactors sync task construction; includes corrected trim section-index mapping.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +1098 to +1102
for (int i = 0; i < trim.length; i++) {
if (trim[i]) {
super.trim(false, i);
}
}
Comment on lines +1092 to +1096
for (int i = 0; i < trim.length; i++) {
if (trim[i]) {
super.trim(false, i);
}
}
Comment on lines +1089 to +1093
for (int i = 0; i < trim.length; i++) {
if (trim[i]) {
super.trim(false, i);
}
}
Comment on lines +1093 to +1097
for (int i = 0; i < trim.length; i++) {
if (trim[i]) {
super.trim(false, i);
}
}
Comment on lines +1085 to +1089
for (int i = 0; i < trim.length; i++) {
if (trim[i]) {
super.trim(false, i);
}
}
Comment on lines +1089 to 1094
}
}
return true;
}

}
Comment on lines +1098 to +1102
for (int i = 0; i < trim.length; i++) {
if (trim[i]) {
super.trim(false, i);
}
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Plugin "stopping" to work.

5 participants