Skip to content

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

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

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

Conversation

@dordsor21

@dordsor21 dordsor21 commented May 24, 2026

Copy link
Copy Markdown
Member

@dordsor21
dordsor21 requested a review from a team as a code owner May 24, 2026 13:21
PierreSchwang
PierreSchwang approved these changes May 25, 2026
View reviewed changes

@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

SirYwell
SirYwell requested changes May 25, 2026
View reviewed changes
Comment thread ...main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v26_1/PaperweightPlatformAdapter.java Outdated
Comment on lines +156 to +160
fieldPendingBlockEntities = ChunkAccess.class.getDeclaredField(Refraction.pickName(
"pendingBlockEntities",
"i"
)
);

@SirYwell SirYwell May 25, 2026

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?

@dordsor21 dordsor21 May 25, 2026

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)

@dordsor21
dordsor21 requested a review from SirYwell May 31, 2026 08:56
@SirYwell SirYwell mentioned this pull request Jun 18, 2026
Open
4 tasks
dordsor21 added 3 commits July 25, 2026 14:32
@dordsor21
fix: improve lock and sync when calling and trimming
8900f89 
 - also fix error when editing newly generated chunks due to postprocessgen not having been completed
 - fixes #3490
@dordsor21
not obf
6d805ad
@dordsor21
add for 26.2
45c46ae
@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.

FastAsyncWorldEdit/worldedit-bukkit/adapters/adapter-26.2/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v26_2/PaperweightGetBlocks.java

Lines 536 to 540 in 45c46ae

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

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

FastAsyncWorldEdit/worldedit-bukkit/adapters/adapter-26.2/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v26_2/PaperweightGetBlocks.java

Lines 1051 to 1056 in 45c46ae

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:

FastAsyncWorldEdit/worldedit-bukkit/adapters/adapter-26.1/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v26_1/PaperweightGetBlocks.java

Lines 534 to 538 in 45c46ae

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).

FastAsyncWorldEdit/worldedit-bukkit/adapters/adapter-1_21/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_21_R1/PaperweightGetBlocks.java

Lines 339 to 343 in 45c46ae

) 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:

FastAsyncWorldEdit/worldedit-bukkit/adapters/adapter-1_21/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_21_R1/PaperweightGetBlocks.java

Lines 632 to 636 in 45c46ae

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

Compare adapter-1_21_11, which has the snapshot:

FastAsyncWorldEdit/worldedit-bukkit/adapters/adapter-1_21_11/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_21_11/PaperweightGetBlocks.java

Lines 336 to 338 in 45c46ae

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 👎.

@MattBDev MattBDev mentioned this pull request Aug 5, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@PierreSchwang PierreSchwang PierreSchwang approved these changes
@SirYwell SirYwell Awaiting requested review from SirYwell

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Plugin "stopping" to work.

4 participants

@dordsor21 @MattBDev @SirYwell @PierreSchwang