search

Globox1997 / BackSlot

https://modrinth.com/mod/backslot
https://www.curseforge.com/minecraft/mc-mods/backslot
GNU General Public License v3.0
9 stars 12 forks source link

Doing server-side packet handling on networking thread which is not thread-safe #107

Closed qouteall closed 1 year ago

qouteall commented 1 year ago

The switch item packet is directly handled in the networking thread. However the packet handling process changes player inventory. So it's not thread-safe. Although it's not thread-safe, relevant bugs can occur very rarely so it usually gets unnoticed.

https://github.com/Globox1997/BackSlot/blob/f6b32510e2eccc463190b844c291153e83b80602/src/main/java/net/backslot/network/BackSlotServerPacket.java#L13

https://github.com/Globox1997/BackSlot/blob/1.19/src/main/java/net/backslot/network/SwitchPacketReceiver.java#L42

The fix is simple, just handle the packet in server.execute(() -> {...})

I noticed this because someone reported that ImmPtl crashes with it https://github.com/iPortalTeam/ImmersivePortalsMod/issues/1323

ImmPtl is just doing thread checking to help debugging. So this is not ImmPtl's issue.

qouteall commented 1 year ago

Fabric API's old networking API design is not good (it does not enforce the handling thread). It's part of the reason. Fabric API introduced the new object-based networking API which is easier to use and can avoid this thread safety issue.

Globox1997 commented 1 year ago

Thanks for the info qouteall. Fixed it :)