Reimplement session structure #869
Conversation
Benefits are clear, developers can easily provide custom behaviour by overriding the server and client classes to provide custom channels (like geyser does for localchannel) and also custom channel handlers can be provided, like for ViaVersion support. Transfers were moved to a flag. There are now ClientSession and ServerSession interfaces which extend Session. This way a ServerSession doesn't have connect methods. Binding to UnixDomainSockets or any other type of SocketAddress is now supported and the client can connect to those too. Tcp was renamed to Net because now non-tcp protocols like local channels and udp (for bedrock ViaBedrock) can be used.
This allows us to share more code between server and client. Additionally, the split worker groups allow mcpl to handle server packets more closely to vanilla and reduce timeouts during load.
…o ipv4 server and vice verse
|
Implementation note: the MinecraftConstants.CLIENT_HOST and CLIENT_PORT flags are dynamic flags used by the ClientSessionListener. If not explicitly set, they'll cast the remote address to an InetSocketAddress to take the values. However for developers using anything other like UnixDomainSockeAddresst or LocalChannelAddress they'll have to set those flags to avoid an cast error. |
| public NetClientSession(SocketAddress remoteAddress, PacketProtocol protocol) { | ||
| this(remoteAddress, null, protocol); | ||
| } | ||
|
|
||
| public NetClientSession(SocketAddress remoteAddress, PacketProtocol protocol, ProxyInfo proxy) { | ||
| this(remoteAddress, null, protocol, proxy); | ||
| } | ||
|
|
||
| public NetClientSession(SocketAddress remoteAddress, SocketAddress bindAddress, PacketProtocol protocol) { | ||
| this(remoteAddress, bindAddress, protocol, null); | ||
| } | ||
|
|
||
| public NetClientSession(SocketAddress remoteAddress, SocketAddress bindAddress, PacketProtocol protocol, ProxyInfo proxy) { | ||
| this(remoteAddress, bindAddress, protocol, proxy, DefaultPacketHandlerExecutor.createExecutor()); | ||
| } | ||
|
|
||
| public NetClientSession(SocketAddress remoteAddress, SocketAddress bindAddress, PacketProtocol protocol, ProxyInfo proxy, Executor packetHandlerExecutor) { | ||
| super(remoteAddress, protocol, packetHandlerExecutor); | ||
| this.bindAddress = bindAddress; | ||
| this.proxy = proxy; | ||
| this.codecHelper = protocol.createHelper(); | ||
| } |
There was a problem hiding this comment.
Thoughts on making these a builder with default values instead of multiple constructors?
example/src/main/java/org/geysermc/mcprotocollib/network/example/PingServerTest.java
Outdated
Show resolved
Hide resolved
protocol/src/main/java/org/geysermc/mcprotocollib/network/helper/NettyHelper.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| protected EventLoopGroup getEventLoopGroup() { | ||
| createEventLoopGroup(); |
There was a problem hiding this comment.
any particular reason the event group null check was moved to the method instead of being here?
There was a problem hiding this comment.
Better isolates EVENT_LOOP_GROUP to only be referenced from one method. Also imo makes this method more readable for devs looking to override this.
protocol/src/main/java/org/geysermc/mcprotocollib/network/net/NetClientSession.java
Outdated
Show resolved
Hide resolved
protocol/src/main/java/org/geysermc/mcprotocollib/protocol/ClientListener.java
Outdated
Show resolved
Hide resolved
protocol/src/main/java/org/geysermc/mcprotocollib/protocol/ClientListener.java
Outdated
Show resolved
Hide resolved
…entListener.java Co-authored-by: chris <[email protected]>
…entListener.java Co-authored-by: chris <[email protected]>
…NetClientSession.java Co-authored-by: chris <[email protected]>
There was a problem hiding this comment.
Code generally looks fine fine to me, but the main nitpick I have is with the naming. net is just an abbreviation of network, and it doesn't make the most sense IMO to have a second network package nested inside of network already. I'd suggest renaming the net package to session to be a bit more clear to the end user.
Additionally I'd suggest the following names:
NetSession->NetworkSessionNetClientSession->ClientNetworkSessionNetServerSession->ServerNetworkSession
I'd drop the Net prefix for all the other remaining classes aside from NetFlowControlHandler being renamed to AutoReadFlowControlHandler or retaining the previous Tcp prefix as the doc indicates this is still for TCP connections. May be better for these classes to be placed in a netty package as well.
|
Alright implemented Redneds advice |
Benefits are clear, developers can easily provide custom behaviour by overriding the server and client classes to provide custom channels (like geyser does for localchannel) and also custom channel handlers can be provided, like for ViaVersion support.
Transfers were moved to a flag. There are now ClientSession and ServerSession interfaces which extend Session. This way a ServerSession doesn't have connect methods. Binding to UnixDomainSockets or any other type of SocketAddress is now supported and the client can connect to those too. Tcp was renamed to Net because now non-tcp protocols like local channels and udp (for bedrock ViaBedrock) can be used.