CraftPlayer.onEntityRemove is not thread safe, crashing server #406
Labels
No labels
documentation
good first issue
help wanted
resolution: duplicate
resolution: invalid
resolution: won't fix
status: needs triage
type: bug
type: feature
type: performance
type: question
version: 1.19.4
version: 1.20
version: 1.20.1
version: 1.20.2
version: 1.20.4
version: 1.20.6
version: 1.21.11
version: 1.21.4
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
Minecraft/Folia#406
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Stack trace
Plugin and Datapack List
plugins: ESU
Actions to reproduce (if known)
ESU is a plugin, which uses Player.hideEntity on player entity scheduler.
onEntityRemove and it may write to the map at the same time.
Folia version
ALL
Other
net.minecraft.server.level.ServerLevel$EntityCallbacks.onTrackingEnd(ServerLevel.java:2989)
Hi. My thought is to add a ConcurrentLinkedList as a queue for each CraftPlayer, which holds the entity uuids to remove, and drain the queue on each player tick.
I can create a PR about this as a temporary fix. Or, do anyone have a better idea?
This is something I was trying to fix earlier today, however was unable to find a practical solution. This is however a very large issue.
The fix proposed here would probably not be the greatest way to resolve this. However the issue stems from the mere usage of the visibility API, and how Folia unsafely accesses this API. It isn't something that should really be synchronized, as that could create insane performance issues, and replacing the collection with a concurrent alternative isn't the greatest idea either. By simply using the API, even from the correct owning context, the server is at risk of crashing randomly whenever an entity is removed due to this unsafe access.
This is indeed a desperate issue and should be fixed ASAP, given the severity of the issue and just USING the API causing risk of this crash, and many popular plugins that are used in production servers use this API.
I believe queue and remove on tick can be a temporary, working solution. (And I'm currently using this solution, and it just works.)
You cannot add a entity to the map (there's a thread check for entities, but not for players I think), that are in different tick threads, so draining the removed queue on tick thread should be thread safe. And entities in different tick threads will not be updated to the player.