Improve injector listener tracking and fix terminal packets breaking injection#2951
Conversation
fix: terminal packets breaking protocol getter on inbound packets
|
I also considered removing some more internal interfaces. Although they provide useful abstraction, they are outdated and poorly maintained. Moreover, they are only used within internal code, making them somewhat obsolete. I wanted to get your opinion on this matter, @derklaro. Here is a list of the interfaces:
The only interfaces I removed so far wouldn't have any functionality in the new system. I would also move some dependencies from methods calls into the constructor such as the |
|
Gonna take a first look later, but I would like to subject to using syncronized over a concurrent map/set. It's just easier and scales better than using syncronized. Just to put it here as well: computeIfAbsent in ConcurrentHashMap locks the map even if the entry already exists in JDK8, should use something else in that case :) |
I don't believe syncronized is an issue in this context because we only synchronize methods that are invoked when a listener is registered or unregistered, which occurs infrequently. Additionally, it is impossible to safely remove an entry from the map without some form of exclusive locking because there's no guarantee that another thread hasn't registered a new listener between the |
| .build()); | ||
| // reset packet to previous packet | ||
| event.setPacket(originalPacket); | ||
| } |
There was a problem hiding this comment.
This method should now handle null packets and complain about naughty plugins.
| PacketContainer packet = subPacketEvent.getPacket(); | ||
| if (packet == null || packet.getHandle() == null) { | ||
| // super.invoke() should prevent us from getting new null packet so we just ignore it here | ||
| continue; |
There was a problem hiding this comment.
This should now be fine without any warning etc.
Description
This PR marks the initial phase of enhancements for the injector, addressing #2919. Additionally, it resolves an issue with inbound terminal packets like ClientIntentionPacket and ServerboundFinishConfigurationPacket. ProtocolLib is unable to retrieve the inbound
PacketType.Protocolfor these packets, a consequence of the PacketDecoder removing itself when a decoded packet is terminal. While indirectly related to #2933 and #2945, this PR distinguishes itself by implementing a (probably) accurate/robust solution for the underlying problem.This PR primarily focuses on revamping the registration process for packet listeners. Instead of maintaining multiple lists of packet listeners per packet type and sets of packet types, we now primarily use two PacketTypeMultiMaps to track the registered packet listeners. I also reworked the collections used to track packet listeners, addressing several issues such as race conditions, performance regressions, and overall readability. The new implementation ensures thread-safe modifications to the collections and uses locks only when necessary, resulting in faster read operations. Additionally, the new code is more readable and accurately represents the current injector.
Related Issue
—
How Has This Been Tested?
Tested with latest spigot 1.20.6 build with latest build of orebfuscator for 1.20.5. I couldn't observe any obvious errors.
Types of Changes