CraftPlayer.onEntityRemove is not thread safe, crashing server

[Rothes]

Stack trace

Region #~ centered at chunk [~, ~] in world 'world_nether' failed to tick:                               
net.minecraft.ReportedException: Exception ticking world                                                                                                                                  
        at net.minecraft.server.MinecraftServer.tickChildren(MinecraftServer.java:1878) ~[server-1.21.8.jar]                                                             
        at net.minecraft.server.MinecraftServer.tickServer(MinecraftServer.java:1693) ~[server-1.21.8.jar]                                                               
        at io.papermc.paper.threadedregions.TickRegions$ConcreteRegionTickHandle.tickRegion(TickRegions.java:409) ~[server-1.21.8.jar]                                   
        at io.papermc.paper.threadedregions.TickRegionScheduler$RegionScheduleHandle.runTick(TickRegionScheduler.java:437) ~[server-1.21.8.jar]                          
        at ca.spottedleaf.concurrentutil.scheduler.SchedulerThreadPool$TickThreadRunner.run(SchedulerThreadPool.java:546) ~[concurrentutil-0.0.3.jar:?]                                   
        at java.base/java.lang.Thread.run(Thread.java:1474) ~[?:?]                                                                                                                        
Caused by: java.lang.ArrayIndexOutOfBoundsException: Index -1 out of bounds for length 2049                                                                                               
        at it.unimi.dsi.fastutil.objects.Object2ObjectOpenHashMap.rehash(Object2ObjectOpenHashMap.java:1281) ~[fastutil-8.5.15.jar:?]                                                     
        at it.unimi.dsi.fastutil.objects.Object2ObjectOpenHashMap.removeEntry(Object2ObjectOpenHashMap.java:209) ~[fastutil-8.5.15.jar:?]                                                 
        at it.unimi.dsi.fastutil.objects.Object2ObjectOpenHashMap.remove(Object2ObjectOpenHashMap.java:308) ~[fastutil-8.5.15.jar:?]                                                      
        at org.bukkit.craftbukkit.entity.CraftPlayer.onEntityRemove(CraftPlayer.java:2212) ~[server-1.21.8.jar]                                                          
        at net.minecraft.server.level.ServerLevel$EntityCallbacks.onTrackingEnd(ServerLevel.java:2989) ~[server-1.21.8.jar]                                              
        at net.minecraft.server.level.ServerLevel$EntityCallbacks.onTrackingEnd(ServerLevel.java:2834) ~[server-1.21.8.jar]                                              
        at ca.spottedleaf.moonrise.patches.chunk_system.level.entity.EntityLookup.entityStatusChange(EntityLookup.java:304) ~[server-1.21.8.jar]                         
        at ca.spottedleaf.moonrise.patches.chunk_system.level.entity.ChunkEntitySlices.updateStatus(ChunkEntitySlices.java:255) ~[server-1.21.8.jar]                     
        at ca.spottedleaf.moonrise.patches.chunk_system.level.entity.EntityLookup.chunkStatusChange(EntityLookup.java:334) ~[server-1.21.8.jar]                          
        at ca.spottedleaf.moonrise.patches.chunk_system.scheduling.NewChunkHolder.changeEntityChunkStatus(NewChunkHolder.java:1212) ~[server-1.21.8.jar]                 
        at ca.spottedleaf.moonrise.patches.chunk_system.scheduling.NewChunkHolder.handleFullStatusChange(NewChunkHolder.java:1310) ~[server-1.21.8.jar]                  
        at ca.spottedleaf.moonrise.patches.chunk_system.scheduling.ChunkHolderManager.processPendingFullUpdate(ChunkHolderManager.java:1637) ~[server-1.21.8.jar]        
        at ca.spottedleaf.moonrise.patches.chunk_system.scheduling.ChunkHolderManager.processTicketUpdates(ChunkHolderManager.java:1606) ~[server-1.21.8.jar]            
        at ca.spottedleaf.moonrise.patches.chunk_system.scheduling.ChunkHolderManager.tick(ChunkHolderManager.java:1031) ~[server-1.21.8.jar]                            
        at net.minecraft.world.level.TicketStorage.purgeStaleTickets(TicketStorage.java:254) ~[server-1.21.8.jar]                                                        
        at net.minecraft.server.level.ServerChunkCache.tick(ServerChunkCache.java:499) ~[server-1.21.8.jar]                                                              
        at net.minecraft.server.level.ServerLevel.tick(ServerLevel.java:791) ~[server-1.21.8.jar]                                                                        
        at net.minecraft.server.MinecraftServer.tickChildren(MinecraftServer.java:1873) ~[server-1.21.8.jar]                                                             
        ... 5 more

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)

            // Folia - region threading - TODO THIS SHIT
            if (!(entity instanceof ServerPlayer)) {
                for (ServerPlayer player : ServerLevel.this.server.getPlayerList().players) { // Paper - call onEntityRemove for all online players
                    player.getBukkitEntity().onEntityRemove(entity);
                }
            }

[Rothes]

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?

[Dueris]

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.

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.

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.

[Rothes]

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.