search

dmulloy2 / ProtocolLib

Provides read and write access to the Minecraft protocol with Bukkit.
GNU General Public License v2.0
987 stars 258 forks source link

Open Questions and Suggestions for Packet Injection System #2919

Open Ingrim4 opened 1 month ago

Ingrim4 commented 1 month ago

Upon reviewing the packet injection system, I've encountered a few points that need discussion and possible changes.

(1) Firstly, I'd like to clarify my understanding: in Netty, a write operation starts at the pipeline tail and progresses towards the head, where the message is eventually written to the outbound buffer or flushed to the operating system kernel, depending on the method used. Given this understanding, I'm curious about the complexity of outbound packet processing, particularly the involvement of NettyEventLoopProxy/processOutbound. Wouldn't it be simpler to incorporate a dedicated channel handler to intercept writes, or to delegate the write and writeAndFlush methods as currently implemented (here)? I've tried to figure out why things are set up the way they are, but I haven't found a good enough reason. So, I'm thinking we should make things simpler. Before I go ahead and create a PR, I'd love to hear what you think about the idea.

(2) Additionally, I've observed that ProtocolLib employs multiple PacketTypeSet instances to track registered listener packet types. However, this approach presents a potential issue: if a plugin chooses to unregister its listeners, all packet types associated with those listeners will also be removed from the packet type sets. This behavior seems incorrect to me. To address this, I propose a potential fix. Nevertheless, I'm open to correction if my understanding is flawed. For reference, the unregister calls occur in the following sequence:

  1. PacketFilterManager::removePacketListener
  2. PacketFilterManager::unregisterPacketListenerInInjectors
  3. NetworkManagerPacketInjector::removePacketHandler
  4. AbstractPacketInjector::removePacketHandler - utilizing the instance from NetworkManagerInjector::inboundListeners
  5. PacketTypeSet::removeType

Additionally paging @derklaro since you have more insights in the new injector

derklaro commented 1 month ago

To (1): Solving the problem with a “proxy” is much easier than adding a handler to the pipeline, because the handler of ProtocolLib must always be at the first position in the pipeline (or at least at the position where a reference to the packet object is still available). This is quite a lot of effort, as you would have to find out somehow whether the position of the handler has changed (also e.g. due to other plugins). In addition, the call to writeAndFlush happens with the proxy on the main thread and not in the Netty EventLoop, so we don't have to put the call to the packet listeners back on the main thread (even if I would like to have a system to actually be able to process packets async). So yes, the system with the proxies is more complicated than just writing a handler and putting it in the channel pipeline, but it makes life a lot easier in other places :)

Regarding (2): That's right, the system has always been broken in this respect. I think it would be great if you improved the registration of listeners in general and then fixed the error directly :)

Hope that helps!

Ingrim4 commented 1 month ago

Understood, seems logical. In addition to the suggested changes, I would also like to rework the async package, which I have had many issues with over the years. Perhaps this could serve as an opportunity to overhaul ProtocolLib's underlying mechanisms to seamlessly handle all processing asynchronously, without requiring users noticing it. This could also open the possibility for reevaluating the current processing order of listeners, although I'm not sure it's necessary at this point.