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

• [x] Make sure you are opening from a topic branch (/feature/fix/docs/ branch (right side)) and not your main branch.
• [x] Ensure that the pull request title represents the desired changelog entry.
• [x] New public fields and methods are annotated with @since TODO.
• [x] 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[bot]]

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

[jquery-package]

Any news on this?