Skip to content

Remove block entities on server thread - #3564

Open
HaHaWTH wants to merge 1 commit into
IntellectualSites:mainfrom
HaHaWTH:fix/te-async-remove
Open

Remove block entities on server thread#3564
HaHaWTH wants to merge 1 commit into
IntellectualSites:mainfrom
HaHaWTH:fix/te-async-remove

Conversation

@HaHaWTH

@HaHaWTH HaHaWTH commented Jun 18, 2026

Copy link
Copy Markdown

Overview

Fix unsafe async block entity removal in Paperweight chunk write adapters.

Description

This PR fixes an issue where PaperweightGetBlocks#internalCall could remove block entities directly while running on FAWE queue worker threads.

Previously, when an edit replaced blocks in a chunk, existing block entities were removed immediately during chunk processing via LevelChunk#removeBlockEntity or the Paper beacon removal path. As the backing blockEntities map is not designed for concurrent writes, this can cause backing map corruption.

Existing Issues

After removing BEs in the final sync tasks, this may spam logs like Block entity A @ BlockPos state Block{B}[facing=north,type=single,waterlogged=false] invalid for ticking: as the removal happens after the chunk section swap.

I am not sure whether the preferred long-term fix is to:

  • keep this approach as the minimal thread-safety fix,
  • move block entity removal into an earlier synchronous phase before the section swap,

Feedback would be appreciated here, especially around the expected ordering between block entity removal, beacon deactivation events, and the chunk section replacement performed by internalCall.

Submitter Checklist

  • Make sure you are opening from a topic branch (/feature/fix/docs/ branch (right side)) and not your main branch.
  • Ensure that the pull request title represents the desired changelog entry.
  • New public fields and methods are annotated with @since TODO.
  • I read and followed the contribution guidelines.

@HaHaWTH
HaHaWTH requested a review from a team as a code owner June 18, 2026 07:39
@SirYwell

Copy link
Copy Markdown
Member

Relevant: #3152

@SirYwell

Copy link
Copy Markdown
Member

Also #3543 I think

@HaHaWTH

HaHaWTH commented Jun 18, 2026

Copy link
Copy Markdown
Author

Workaround for log spam: set the remove field through reflection inside the loop, this should be a minimal fix

@HaHaWTH

HaHaWTH commented Jun 18, 2026

Copy link
Copy Markdown
Author

Reference for event contract: BlockEntity removal should happen after the event
image
image

@dordsor21

Copy link
Copy Markdown
Member

Is it not the case that we need to remove the tiles before we perform the edit? Do ticks occurring mid-edit not have an issue if we've replaced the tile in a chunk section it then attempts to do something with

@HaHaWTH

HaHaWTH commented Jul 25, 2026

Copy link
Copy Markdown
Author

Is it not the case that we need to remove the tiles before we perform the edit? Do ticks occurring mid-edit not have an issue if we've replaced the tile in a chunk section it then attempts to do something with

Yes, that's what I'm talking about in the Existing Issues section, because Paper has validation checks before block entity ticking, this won't cause unexpected behavior but spam logs like Block entity A @ BlockPos state Block{B}[facing=north,type=single,waterlogged=false] invalid for ticking:.

@dordsor21

Copy link
Copy Markdown
Member

Yes, that's what I'm talking about in the Existing Issues section, because Paper has validation checks before block entity ticking, this won't cause unexpected behavior but spam logs like Block entity A @ BlockPos state Block{B}[facing=north,type=single,waterlogged=false] invalid for ticking:.

Yep I see. w.r.t #3152 this is fixed by putting the sync before the rest of the function - I would still request the same as I did in that PR where we should be chaining the future(s) together (if not already on main thread) similar to how it is handled for if we do/do not yet have the chunk loaded

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.

3 participants