Out/In bound protocol injection improvements

[derklaro]

This pull request introduces quite some changes to the way protocol lib handles and sends packets to the clients.

Current stuff which isn't done yet completely:

• make injection into the channel more sane, it's current a bit harsh regarding field overrides...

Some changes I made to the api:

• I removed some very old api, for example the socket based injection and byte-array based packet interception. These changes are mainly done because there is no good way to do both, good injection and supporting api that nobody should use as we provide way better and cleaner ways to intercept a packet (WirePackets are still a thing though).
• I removed some exception declarations from api methods, for example the InvocationTargetException from sendServerPacket. It requires the api user to catch these exceptions while they are never thrown by the underlying code and are arguably relicts from ancient days.
• There is no SocketInjector anymore (which provided access to a Socket which wrapped the incoming channel), mainly as that api was probably unused by most users and was already broken, depending which server software a user was using.
• Theres maybe more breakages if a user was using non-api methods...

But this pull request also includes some very nice changes:

• ProtocolLib no longer wraps the en/decoder of minecraft, instead it hooks into the pipeline allowing other injectors to get into it far more easily. (The general way on how we intercepted outbound packets is still the same, we're proxying the channel field and read the packets before they are actually written into the pipeline. This has 2 main bright sides:
  • there is no need for netty to excute something in the pipeline if the outbound packet is cancelled anyway
  • packet listeners which explicitly require tasks on the main thread will be easier to call as the invocation of write / writeAndFlush will always be on the main thread (on the vanilla server at least).
• Explicit uninjection from a player is now a thing
• A very much overall performance improvement by removing as much as reflection as possible from the main thread and replacing very old code without a need anymore (which was still executed!)
• Maybe not the biggest thing, but the code is much cleaner and more easy to maintain and update for further versions of ProtocolLib!

Just a few more notes I want to conclude this with:

• This change is heavily experimental, but I did quite a few test
  • performance tests [spawning hundreds of mobs which are moving around], results:
    • https://spark.lucko.me/WhT85eGeTV
    • https://spark.lucko.me/b8tor1480g
    • https://timings.aikar.co/?id=fade4226b5964752a05087af8d5831d0
  • Packet interception tests (including sending of WirePackets), modifying inbound/outbound packets, sending custom ones, cancelling them etc.
  • general working tests, for example injector removal on disconnect
• Other than that I think a separate branch to work further would be really nice, as there are quite a lot of other things which can be improved which I didn't want to include in this pull request as the diff is already massive and more than enough imho 😂

I think this pull request resolves any Issue regarding performance and/or how we inject into the channel pipeline! Love to get some feedback on this from everybody who previously reported such issues 😀

I would no recommend anyone to use this in production as this change might still contain bugs

[dmulloy2]

Looks great! Thank you for your work on this!

My thinking is to release 4.8.0 after merging in 1.18.2 support, bump the version to 5.0.0, then release this on jenkins for some testing. Metrics would imply that at least most people aren't using the bleeding edge builds: https://bstats.org/plugin/bukkit/ProtocolLib/1453

[derklaro]

Sounds good 👍

[Rothes]

And is it possible to use ReflectASM for better performance?

[derklaro]

Well, not really (I guess)... I can recheck if that is possible but from the code I see the library will not be able to override "trusted" final fields, as for example used in records.

Furthermore, I don't like that the lib doesn't get any "real" updates, for example the asm library dependency is 5 years out of date...

[retrooper]

Looks great! Thank you for your work on this!

My thinking is to release 4.8.0 after merging in 1.18.2 support, bump the version to 5.0.0, then release this on jenkins for some testing. Metrics would imply that at least most people aren't using the bleeding edge builds: https://bstats.org/plugin/bukkit/ProtocolLib/1453

I guess we can bump it now?

[derklaro]

@dmulloy2 was the merge into master rather than dev intentional?

[dmulloy2]

yeah im just messing around with formatting and linting on dev

[derklaro]

ah i see 👍

[retrooper]

Love you guys <3

[retrooper]

Now we are just waiting for ViaVersion to do the same... But they might take much longer.

[matipoirierg]

Hi, is this already on latest dev builds? I'd love to do some testing

[derklaro]

Yes

[matipoirierg]

Yes

thanks

[Dreig-Michihi]

What to do with INTERCEPT_INPUT_BUFFER removing?

[Dreig-Michihi]

Why did you remove this enum? What to use instead?

What about the method
com.comphenix.protocol.events.PacketEvent.getNetworkMarker()
throwing exception, which asks to "Add the option ListenerOptions.INTERCEPT_INPUT_BUFFER to your listener."
@derklaro
@dmulloy2

[derklaro]

There is no replacement for this. See my initial PR comment for details