Fix crash due to data race when removing block entity

[SirYwell]

Overview

Fixes #3151

Description

This is rather a quick fix for the server crash. Performance impact seems to be okay, but we might want to take a closer look in future. We could probably also use Minecraft's new tick freezing mechanism instead. The beacon special-casing can be removed too probably.

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.

[dordsor21]

Is it better worth to locate the tiles async and only perform the removes synchronously, if required, rather than needing a (potentially pointless) main-thread task on every chunk call?

[SirYwell]

Using the map returned by getBlockEntities() in any way isn't thread-safe to begin with, so I'm not sure if we can skip it at all. If we have GET data already we could check if any of those blocks has a block entity, but that could also be out-of-date information...

[dordsor21]

Using the map returned by getBlockEntities() in any way isn't thread-safe to begin with, so I'm not sure if we can skip it at all. If we have GET data already we could check if any of those blocks has a block entity, but that could also be out-of-date information...

If the map is empty when we retrieve it I think it's acceptable to skip the main thread call entirely tbh. Otherwise yes I suppose it probably is best to iterate the map in the sync job. Also, with the previous changes made to the futures, I wonder if we want do something similar to how we handle the chunk load waiting by returning a future for the main thread operation, which continues with the operation when complete

[SirYwell]

If the map is empty when we retrieve it I think it's acceptable to skip the main thread call entirely tbh. Otherwise yes I suppose it probably is best to iterate the map in the sync job.

Checking whether the map is empty asynchronously is already flawed, so I don't think we can skip that.

Also, with the previous changes made to the futures, I wonder if we want do something similar to how we handle the chunk load waiting by returning a future for the main thread operation, which continues with the operation when complete

I was thinking about that too, but that makes it more difficult to get things back to the threads we want to run it on, so I decided to go the easiest route for now. Maybe we should think about switching to e.g. an actor model in future, that would allow far more flexibility in such scenarios.

[dordsor21]

Checking whether the map is empty asynchronously is already flawed, so I don't think we can skip that.

Arguably the same can be said for the whole operation at that point. The rest of FAWE assumes the chunk isn't modified after reading (cached GET for masks could be cached a while) and halting everything awaiting a potentially empty main thread task feels suboptimal.

I was thinking about that too, but that makes it more difficult to get things back to the threads we want to run it on, so I decided to go the easiest route for now. Maybe we should think about switching to e.g. an actor model in future, that would allow far more flexibility in such scenarios.

Currently after the chunk async load we just "resubmit" the task and return that future. Here we would submit a main thread task instead, which after completion would perform the resubmission to the FAWE queue(s)

[github-actions]

Please take a moment and address the merge conflicts of your pull request. Thanks!

[Copilot]

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (5)

worldedit-bukkit/adapters/adapter-1_21_4/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_21_4/PaperweightGetBlocks.java:349

• Consider initializing beaconEntities with an empty ArrayList (e.g., new ArrayList<>()) rather than null to ensure a non-null return value, which can improve null-safety for later processing.

List<BlockEntity> beaconEntities = null;

worldedit-bukkit/adapters/adapter-1_21_3/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_21_3/PaperweightGetBlocks.java:349

• Consider initializing beaconEntities with an empty ArrayList immediately to avoid returning null if no beacon entities are found.

List<BlockEntity> beaconEntities = null;

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

• Consider initializing beaconEntities to new ArrayList<>() instead of null to ensure that the method returns a valid (even if empty) list.

List<BlockEntity> beaconEntities = null;

worldedit-bukkit/adapters/adapter-1_20_5/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_20_R4/PaperweightGetBlocks.java:348

• Initialize beaconEntities with an empty ArrayList right away to avoid the potential for a null pointer later in the code.

List<BlockEntity> beaconEntities = null;

worldedit-bukkit/adapters/adapter-1_20_2/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_20_R2/PaperweightGetBlocks.java:347

• It is recommended to initialize beaconEntities as a new ArrayList<>() immediately rather than null to avoid null-related issues downstream.

List<BlockEntity> beaconEntities = null;

[Copilot]

Consider initializing beaconEntities to new ArrayList<>() instead of null so that the method consistently returns a non-null list.

Suggested change

	List<BlockEntity> beaconEntities = null;
	List<BlockEntity> beaconEntities = new ArrayList<>();

[jquery-package]

Any news on this?