Improper threading on Folia

[Dueris]

I am unsure about all of the places where this instance occurs, however I got a report in my discord about a thread check triggered by this plugin. The cause of this is because this plugin attempts to open an inventory on the global tick in Folia, however this is not safe.

For context, I own CanvasMC, a fork of Folia aiming to try and be more stable and consistent region threading. In my fork, we introduce a few new thread checks on places that should be guarded to ensure region threading safety is kept. Inventory opening/closing is one of these things that Ive seen improperly scheduled, hence why a check was placed there.

The global tick should never handle the opening/closing of inventories, this should always be scheduled at the player. This is capable of causing race conditions and such within the server that are frankly, undiagnosable, even on Folia.

[22:58:46 INFO]: EuphylliaBierque issued server command: /openinv _Euphyllia
[22:58:46 ERROR]: [ca.spottedleaf.moonrise.common.util.TickThread] Thread failed main thread check: Cannot init menu async, context=[thread=Folia Region Scheduler Thread #0,class=io.papermc.paper.threadedregions.TickRegionScheduler$TickThreadRunner,region={null}], entity={root=[{type=ServerPlayer,id=1,uuid=246e3682-864e-48a9-a1d0-12b27362f13e,pos=(34.782,66.619,4.157),mot=(0.000,0.000,0.000),aabb=AABB[34.482383351523794, 66.61949008752146, 3.8572115181502378] -> [35.08238337536565, 68.41949003983774, 4.457211541992096],removed=null,has_vehicle=false,passenger_count=0], vehicle=[{null}], passengers=[]
java.lang.Throwable
        at ca.spottedleaf.moonrise.common.util.TickThread.ensureTickThread(TickThread.java:97) ~[tensei-26.1.2.jar:26.1.2--1-5627adb]
        at io.canvasmc.canvas.util.TickGuard.guard(TickGuard.java:46) ~[tensei-26.1.2.jar:26.1.2--1-5627adb]
        at net.minecraft.server.level.ServerPlayer.initMenu(ServerPlayer.java:913) ~[tensei-26.1.2.jar:26.1.2--1-5627adb]
        at OpenInv.jar//com.lishid.openinv.internal.paper26_1.player.PlayerManager.openInventory(PlayerManager.java:312) ~[?:?]
        at OpenInv.jar//com.lishid.openinv.util.InternalAccessor.openInventory(InternalAccessor.java:271) ~[?:?]
        at OpenInv.jar//com.lishid.openinv.OpenInv.openInventory(OpenInv.java:305) ~[?:?]
        at OpenInv.jar//com.lishid.openinv.command.OpenInvCommand.handle(OpenInvCommand.java:176) ~[?:?]
        at OpenInv.jar//com.lishid.openinv.command.PlayerLookupCommand$1$1.run(PlayerLookupCommand.java:85) ~[?:?]
        at OpenInv.jar//com.github.jikoo.openinv.lib.nahu.scheduler-wrapper.implementation.folia.FoliaWrappedScheduler.lambda$runTask$1(FoliaWrappedScheduler.java:93) ~[?:?]
        at io.papermc.paper.threadedregions.scheduler.FoliaGlobalRegionScheduler$GlobalScheduledTask.run(FoliaGlobalRegionScheduler.java:178) ~[tensei-26.1.2.jar:?]
        at io.papermc.paper.threadedregions.scheduler.FoliaGlobalRegionScheduler.tick(FoliaGlobalRegionScheduler.java:36) ~[tensei-26.1.2.jar:?]
        at io.papermc.paper.threadedregions.RegionizedServer.globalTick(RegionizedServer.java:348) ~[tensei-26.1.2.jar:26.1.2--1-5627adb]
        at io.papermc.paper.threadedregions.RegionizedServer$GlobalTickTickHandle.tickRegion(RegionizedServer.java:192) ~[tensei-26.1.2.jar:26.1.2--1-5627adb]
        at io.papermc.paper.threadedregions.TickRegionScheduler$RegionScheduleHandle.runTick(TickRegionScheduler.java:524) ~[tensei-26.1.2.jar:26.1.2--1-5627adb]
        at io.canvasmc.canvas.tick.AffinitySchedulerThreadPool$TickThreadRunner.run(AffinitySchedulerThreadPool.java:625) ~[tensei-26.1.2.jar:26.1.2--1-5627adb]
        at java.base/java.lang.Thread.run(Thread.java:1474) ~[?:?]
[22:58:46 WARN]: [OpenInv] Global task for OpenInv v5.3.2-SNAPSHOT generated an exception
java.lang.IllegalStateException: Thread failed main thread check: Cannot init menu async, context=[thread=Folia Region Scheduler Thread #0,class=io.papermc.paper.threadedregions.TickRegionScheduler$TickThreadRunner,region={null}], entity={root=[{type=ServerPlayer,id=1,uuid=246e3682-864e-48a9-a1d0-12b27362f13e,pos=(34.782,66.619,4.157),mot=(0.000,0.000,0.000),aabb=AABB[34.482383351523794, 66.61949008752146, 3.8572115181502378] -> [35.08238337536565, 68.41949003983774, 4.457211541992096],removed=null,has_vehicle=false,passenger_count=0], vehicle=[{null}], passengers=[]
        at ca.spottedleaf.moonrise.common.util.TickThread.ensureTickThread(TickThread.java:98) ~[tensei-26.1.2.jar:26.1.2--1-5627adb]
        at io.canvasmc.canvas.util.TickGuard.guard(TickGuard.java:46) ~[tensei-26.1.2.jar:26.1.2--1-5627adb]
        at net.minecraft.server.level.ServerPlayer.initMenu(ServerPlayer.java:913) ~[tensei-26.1.2.jar:26.1.2--1-5627adb]
        at OpenInv.jar//com.lishid.openinv.internal.paper26_1.player.PlayerManager.openInventory(PlayerManager.java:312) ~[?:?]
        at OpenInv.jar//com.lishid.openinv.util.InternalAccessor.openInventory(InternalAccessor.java:271) ~[?:?]
        at OpenInv.jar//com.lishid.openinv.OpenInv.openInventory(OpenInv.java:305) ~[?:?]
        at OpenInv.jar//com.lishid.openinv.command.OpenInvCommand.handle(OpenInvCommand.java:176) ~[?:?]
        at OpenInv.jar//com.lishid.openinv.command.PlayerLookupCommand$1$1.run(PlayerLookupCommand.java:85) ~[?:?]
        at OpenInv.jar//com.github.jikoo.openinv.lib.nahu.scheduler-wrapper.implementation.folia.FoliaWrappedScheduler.lambda$runTask$1(FoliaWrappedScheduler.java:93) ~[?:?]
        at io.papermc.paper.threadedregions.scheduler.FoliaGlobalRegionScheduler$GlobalScheduledTask.run(FoliaGlobalRegionScheduler.java:178) ~[tensei-26.1.2.jar:?]
        at io.papermc.paper.threadedregions.scheduler.FoliaGlobalRegionScheduler.tick(FoliaGlobalRegionScheduler.java:36) ~[tensei-26.1.2.jar:?]
        at io.papermc.paper.threadedregions.RegionizedServer.globalTick(RegionizedServer.java:348) ~[tensei-26.1.2.jar:26.1.2--1-5627adb]
        at io.papermc.paper.threadedregions.RegionizedServer$GlobalTickTickHandle.tickRegion(RegionizedServer.java:192) ~[tensei-26.1.2.jar:26.1.2--1-5627adb]
        at io.papermc.paper.threadedregions.TickRegionScheduler$RegionScheduleHandle.runTick(TickRegionScheduler.java:524) ~[tensei-26.1.2.jar:26.1.2--1-5627adb]
        at io.canvasmc.canvas.tick.AffinitySchedulerThreadPool$TickThreadRunner.run(AffinitySchedulerThreadPool.java:625) ~[tensei-26.1.2.jar:26.1.2--1-5627adb]
        at java.base/java.lang.Thread.run(Thread.java:1474) ~[?:?]

As you can see from the stacktrace, the PlayerManager class for Paper 26.1 calls initMenu, which is unsafe to execute in parallel from the owning context of the player.

OpenInv/internal/paper26_1/src/main/java/com/lishid/openinv/internal/paper26_1/player/PlayerManager.java

Line 255 in 8f058fb

public @Nullable InventoryView openInventory(

This realistically should be changed alongside other places in the source code if applicable to adjust to support Folia more safely. While this may not throw an exception on Folia like this exactly, it is a Folia issue, just harder to catch, and not really something diagnosable.

[Jikoo]

This looks like one of the major reservations I had with Folia:

The way OI works may require a lot more modification under the hood. Currently OI provides a view backed by the same in-memory contents of the player's inventory, which removes the need (in a normal server) to synchronize contents to ensure that inventories are up-to-date and prevent duplications/deletions. I have no idea how Folia handles inventories opened by multiple players in different regions, but actions would have to be synced to the region owning the player (if any, see note above about constructing a ServerPlayer being a complication) to prevent duplications/item corruption/general inventory functionality breakdown by concurrent modifications of the same backing contents list from different threads.

How does Folia handle the same inventory being opened by different players in different regions?

Vaguely related Folia thoughts/reservations in #132, Folia support added in #292
cc @xCodiq and @R00tB33rMan

[Dueris]

Folia doesn't handle that. The internals of the inventory aren't thread safe at all, and neither is opening/closing inventories not on the player context

[Jikoo]

Folia doesn't handle that. The internals of the inventory aren't thread safe at all, and neither is opening/closing inventories not on the player context

Hmm. I see why that'd have to be the case in terms of the lower inventory, though it is a bit frustrating that Folia's design essentially disallows most remote access or trading plugins without an additional layer. Is there a way to signal to Folia that two regions should be merged? It doesn't look like there is, based on the docs, but I admittedly don't follow Folia development.

Sounds like OI is fundamentally incompatible with Folia and forks until the backing access system is rewritten. The scheduler bandaid is acceptable by me because I think the actual risk of concurrent modification is negligible, but when I do get to writing a generic Bukkit fallback I'll definitely be thinking about Folia with it.

[rautamiekka]

Best complain directly to Folia, then, otherwise nothing is gonna change cuz no one will know.