fabric icon indicating copy to clipboard operation
fabric copied to clipboard

Network API Docs should tell people to use correct threads

Open frqnny opened this issue 2 years ago • 2 comments

To quote from quoteall,

I have something to say about the networking API's documentation. The method registerGlobalReceiver 's doc should explicitly say that the handler is invoked on networking thread which should not manipulate the server world directly. (Although the doc of PlayChannelHandler mentions that, it's not enough, this is a very important information and should be said many times). Many mods manipulate the server entities in networking thread which is unsafe (and Immersive Portals will check that and crash). Recently as far as I know Dual Wielding and AdventureZ have these issues.

We should tell people to use server#execute and client#execute when necessary.

frqnny avatar Dec 10 '21 03:12 frqnny

A good implementation should read the data from the buffer in the networking thread and then manipulate the world in client or server thread.

qouteall avatar Dec 10 '21 03:12 qouteall

I agree. This is also a very common mistake, that I've seen modders make dating back years. i.e. CodeChicken's mob dismemberment mod made this mistake and the result was that dismembering large numbers of entities could cause a crash due to concurrent modification exceptions.

It's the exact same error that I encountered in the exact same way when spawning large numbers of items until I realised it was because everything server-side was being done on the networking thread. :'D

Also to add to that, errors on the network thread can be extremely hard to debug, as they sometimes may not even get logged, rather being silently consumed by the networking handler, so yet another reason to always use client.execute or server.execute

Sollace avatar Dec 11 '21 15:12 Sollace

Fixed by introducing the packet-object API: #2820.

Technici4n avatar May 06 '23 09:05 Technici4n