Proposal to change async packet processing logic#2503
Proposal to change async packet processing logic#2503dmulloy2 merged 3 commits intodmulloy2:masterfrom
Conversation
|
@derklaro thoughts on this? |
derklaro
left a comment
There was a problem hiding this comment.
Based on the description and by looking at the code this propasal lgtm. From the description I also got a feeling that using a more reactive approach here would be way better (kinda like netty does, the handler returns a future and when the future returns the next handler is called and so on - another approach would be via a context that knows the next handler to call). This would give plugins the ability to process the packets in their own scheduler system and take their time before passing to the next handler, and even cancelling would be easy: just don't pass it to the next handler 🤷
But this is something for later, for now this is fine 😄
|
This PR causes issues with Orebfuscator. When there is lot of players online (150+) and they all start to load chunks (for example event spawned in world), chunk loading is VERY VERY slow. It can took ~20 seconds till new chunk will be sent to player. Also when teleporting to spawn where ALL chunks are already loaded it can took up to ~30 seconds for chunks to appear. Reverting this PR resolves this issue. @Ingrim4 can you look into this. |
|
This PR can't be reverted as it would brake any two plugins that use async listeners for the same packet type. I can however look further into the issues you're experiencing. Can you please open a separate issue one Imprex-Development/orebfuscator with a ProtocolLib and Orebfuscator dump file attached? Please also run the |
I got recently confronted with an incompatibility between my plugin Orebufscator and RealisticSeasons. Both plugins are using async packet processing and delay the packet events (increase the processing delay). The way multiple async packet listeners are currently handled is as follows:
This means if the first listener increases the processing delay then the second listener will process the same packet in parallel. The only way listeners can acquire exclusive access to a packet is using the
processingLockobject of the packet'sAsyncMarker. This way of handling multi-threaded access to the packet event isn't very convenient since it could lead to unnecessary locked threads (if a listener would hold the lock for the entire processing step) and potential deadlocks if multiple packets are needed to process a single packet. This is especially harmful to the performance of plugins that process the packet in their own scheduler system (Orebfuscator does that because we need to jump back and forth between processing and main thread in some instances) because this could clog up the entire scheduler (particularly if a plugin needs a long time to process a packet). Furthermore there is currently a way for badly written plugins to prematurely send a packet through multiple decreasings of the processing delay without given other listeners time to process the packet.That's why I propose the simple changes in this PR to effectively change steps 4 and 6 to also process the packet async (with processing delay reduced to zero) before calling the next listener. This way only one listener at a time can process a packet meaning the packet would always be "stable". Another way of solving this problem would be a new method for a listeners to explicitly tell ProtocolLib to only process this packet "sequentially" in that only this listener can process it and other listeners can after it's done. I would also be open for other solutions and would gladly implement them.