Split incoming/outgoing packet registry, transition protocol states correctly #841

AlexProgrammerDE · 2024-07-20T20:35:00Z

These changes are necessary to avoid race-conditions in the code and to prepare the codebase for ViaVersion support.

…tion

AlexProgrammerDE · 2024-07-26T08:17:25Z

PR is ready for review

Konicai

nice, especially like the cleanup in the listeners

protocol/src/main/java/org/geysermc/mcprotocollib/network/packet/Packet.java

protocol/src/main/java/org/geysermc/mcprotocollib/network/Session.java

protocol/src/main/java/org/geysermc/mcprotocollib/network/tcp/TcpFlowControlHandler.java

protocol/src/main/java/org/geysermc/mcprotocollib/protocol/ClientListener.java

Konicai · 2024-07-28T06:51:01Z

protocol/src/main/java/org/geysermc/mcprotocollib/protocol/ServerListener.java

+                // The developer who sends ClientboundStartConfigurationPacket needs to setOutboundState to CONFIGURATION
+                // after sending the packet. We can't do it in this class because it needs to be a method call right after it was sent.
+                // Using nettys event loop to change outgoing state may cause differences to vanilla.


can we not check for ClientboundStartConfigurationPacket in packetSent, and setOutboundState to CONFIGURATION there? packetSent should be invoked more or less right after the onSent callback would be?

I'd rather have the developers explicitly switch them like in vanilla. The entire onSent thing is made because of that reason. Stuff in packetSent may be not be handled immediately because packets sometimes get handled on the event loop. However onSent is guaranteed to be handled inside the netty pipeline.

ClientboundStartConfigurationPacket is not isPriority
therefore it gets handled on the event loop
so if you
send(ClientboundStartConfigurationPacket)
and
send(SomeConfigurationPacket)
as a developer right after each other, it'll error because the event loop is having a race condition with the netty pipeline
One other fix would be setting ClientboundStartConfigurationPacket as isPriority, but I don't wanna touch that with this PR
onSent allows the dev to change the state the next packets gets decoded with immediately without the chance of race conditions because of off-pipeline processing

I'm sorry if I don't understand.

Stuff in packetSent may be not be handled immediately because packets sometimes get handled on the event loop. However onSent is guaranteed to be handled inside the netty pipeline.

I read them as being called right after eachother? https://github.com/AlexProgrammerDE/MCProtocolLib/blob/2b310d2618f23127640a85194623126b1eef594b/protocol/src/main/java/org/geysermc/mcprotocollib/network/tcp/TcpSession.java#L271-L275

Also, isPriority doesn't seem to apply to outbound packets?

Ah yeah you're right. I was thinking too much from the clients perspective that all packets are in the event loop. Would still be weird though to add back packetSent...
It makes it harder to compare logic with vanilla.

It does have the distinction now that any packets handled by ServerListener in the packetSent event are packets that have been sent by the library user, not internal MCPL. I think its worth it to not introduce state switching to the library user for a single case (if i understand correctly)

This is now not even possible anymore since explicit flushes after the writeAndFlush are needed due to race conditions. The code doesn't use onSent anymore for state switching. Developers need to manually switch state after sending the packet.

Would still be weird though to add back packetSent...
It makes it harder to compare logic with vanilla.

Minecraft is also the maintainer AND user of its network code—I think it is worth it here for the sake of ease of use.

Minecraft has a method for sending ClientboundStartConfigurationPacket and switching outbound protocol, but I'm not sure this project's structure would facilitate something like that?

protocol/src/main/java/org/geysermc/mcprotocollib/protocol/ServerListener.java

…uring a task

protocol/src/main/java/org/geysermc/mcprotocollib/network/Session.java

protocol/src/main/java/org/geysermc/mcprotocollib/network/tcp/TcpPacketCodec.java

protocol/src/main/java/org/geysermc/mcprotocollib/protocol/MinecraftProtocol.java

protocol/src/main/java/org/geysermc/mcprotocollib/protocol/ServerListener.java

…ion.java Co-authored-by: chris <github@onechris.mozmail.com>

…ecraftProtocol.java Co-authored-by: chris <github@onechris.mozmail.com>

This replicates the same approach Mojang uses in their networking code.

This reverts commit 7f8affb.

This reverts commit 4e2b64d.

This reverts commit 6507e77.

AlexProgrammerDE · 2024-09-12T11:07:17Z

Race conditions and weird disconnects are fixed. This PR can be merged.

protocol/src/main/java/org/geysermc/mcprotocollib/network/tcp/TcpClientSession.java

protocol/src/main/java/org/geysermc/mcprotocollib/network/tcp/TcpServer.java

protocol/src/main/java/org/geysermc/mcprotocollib/network/tcp/TcpClientSession.java

protocol/src/main/java/org/geysermc/mcprotocollib/network/tcp/TcpPacketCodec.java

protocol/src/main/java/org/geysermc/mcprotocollib/network/tcp/DropPacketFilter.java

…TcpServer.java Co-authored-by: chris <github@onechris.mozmail.com>

Konicai · 2024-09-13T19:42:00Z

protocol/src/main/java/org/geysermc/mcprotocollib/protocol/ServerListener.java

+                // The developer who sends ClientboundStartConfigurationPacket needs to setOutboundState to CONFIGURATION
+                // after sending the packet. We can't do it in this class because it needs to be a method call right after it was sent.
+                // Using nettys event loop to change outgoing state may cause differences to vanilla.


Would still be weird though to add back packetSent...
It makes it harder to compare logic with vanilla.

Minecraft is also the maintainer AND user of its network code—I think it is worth it here for the sake of ease of use.

Minecraft has a method for sending ClientboundStartConfigurationPacket and switching outbound protocol, but I'm not sure this project's structure would facilitate something like that?

Konicai · 2024-09-13T19:55:26Z

protocol/src/main/java/org/geysermc/mcprotocollib/protocol/ServerListener.java

-                if (keepAlivePacket.getPingId() == this.lastPingId) {
-                    long time = System.currentTimeMillis() - this.lastPingTime;
-                    session.setFlag(MinecraftConstants.PING_KEY, time);
+                KeepAliveState currentKeepAliveState = this.keepAliveState;


What specifically is this being cached to a variable for? The field value seems to be mutated when updating the keep alive, but replaced on login and config start/end.

If we are trying to prevent races with the keep alive thread, have we considered making KeepAliveState immutable?

Also, could keepAlivePending be replaced with the field being null when not pending, and have the field be volatile?

This is coded closer to mojangs impl.
The pending field and other fields are part of the commonpacketlistener. And each protocol state keeps track of separate keepalives.

protocol/src/main/java/org/geysermc/mcprotocollib/protocol/ServerListener.java

# Conflicts: # protocol/src/main/java/org/geysermc/mcprotocollib/protocol/ClientListener.java # protocol/src/main/java/org/geysermc/mcprotocollib/protocol/ServerListener.java

Camotoy · 2024-09-19T16:52:30Z

protocol/src/main/java/org/geysermc/mcprotocollib/protocol/MinecraftProtocol.java

@@ -84,7 +90,8 @@ public MinecraftProtocol(PacketCodec codec) {
        this.codec = codec;
        this.targetState = ProtocolState.STATUS;

-        this.setState(ProtocolState.HANDSHAKE);
+        this.setInboundState(ProtocolState.HANDSHAKE);


Seems like we could just have a setState/setStates method just for convenience.

protocol/src/main/java/org/geysermc/mcprotocollib/network/tcp/TcpClientSession.java

Initial code changes

b6a7b0a

AlexProgrammerDE marked this pull request as draft July 20, 2024 20:35

AlexProgrammerDE added 4 commits July 20, 2024 23:18

Make it compile

0d18334

Small inlining

9f10790

Make less detectable by anticheats and fix keepalive during configura…

def403a

…tion

Fix keepalive edge case

f2d67ef

AlexProgrammerDE marked this pull request as ready for review July 26, 2024 07:52

Properly switch inbound protocol in server listener

e33042c

AlexProgrammerDE added 3 commits July 26, 2024 11:14

Add flow control

1341744

Make toggling automatic keepalive work in another way

fccce03

Remove ping pong packets again

a7e539f

Konicai requested changes Jul 28, 2024

View reviewed changes

Konicai changed the title ~~Implement incoming/outgoing packet registry spliitting to exactly replicate vanilla behaviour.~~ Split incoming/outgoing packet registry, transition protocol states correctly Jul 28, 2024

AlexProgrammerDE added 3 commits July 28, 2024 09:40

Address review

2b310d2

Handle keepalive in configuration

72721b8

Only spawn keepalive after login is acknowledged

576b311

AlexProgrammerDE requested a review from Konicai July 28, 2024 08:39

AlexProgrammerDE added 3 commits July 28, 2024 11:57

Prevent very unlikely race conditions with keepalive being switched d…

68a426d

…uring a task

Add debug log for packet serialization and state switching

2ae3b2f

Add one more debug print

5d90ce1

onebeastchris approved these changes Aug 20, 2024

View reviewed changes

AlexProgrammerDE and others added 6 commits August 20, 2024 12:42

Update protocol/src/main/java/org/geysermc/mcprotocollib/network/Sess…

2439c69

…ion.java Co-authored-by: chris <github@onechris.mozmail.com>

Update protocol/src/main/java/org/geysermc/mcprotocollib/protocol/Min…

087e644

…ecraftProtocol.java Co-authored-by: chris <github@onechris.mozmail.com>

Update protocol/src/main/java/org/geysermc/mcprotocollib/protocol/Min…

608b03d

…ecraftProtocol.java Co-authored-by: chris <github@onechris.mozmail.com>

Mark packet as nonnull

b54a21b

Merge remote-tracking branch 'origin/splitting-v3' into splitting-v3

618c0b4

Fix outbound writing race conditions

324f066

Redned235 approved these changes Sep 8, 2024

View reviewed changes

Ensure packets are always sent on the event loop

0ff09d1

This replicates the same approach Mojang uses in their networking code.

AlexProgrammerDE added 12 commits September 11, 2024 23:55

Revert "Make instance dynamic"

3111bd1

This reverts commit 7f8affb.

Make flush packet priority

9706bb3

Do not hide original line that is the cause of the exception

d4eddfa

Cancel packet using exception rather than return

6507e77

Properly iterate through parents

4e2b64d

Set log level to debug for unit tests

e82133d

Revert "Properly iterate through parents"

54228e5

This reverts commit 4e2b64d.

Revert "Cancel packet using exception rather than return"

c57de2a

This reverts commit 6507e77.

Add write length filter

d84ce87

Reuse bytebuf for fake flush to avoid unnecessary allocations

793dfae

Make tests happy

187e2c5

Remake dropping packets

615cec8

onebeastchris reviewed Sep 12, 2024

View reviewed changes

AlexProgrammerDE and others added 4 commits September 12, 2024 22:14

Update protocol/src/main/java/org/geysermc/mcprotocollib/network/tcp/…

792519a

…TcpServer.java Co-authored-by: chris <github@onechris.mozmail.com>

Fix space

a12a952

Rename to flush packet

f9b86e5

Add mojmap reference

0b28bf7

Konicai reviewed Sep 13, 2024

View reviewed changes

AlexProgrammerDE added 5 commits September 14, 2024 00:44

Share keepalive code

e4976d9

Merge remote-tracking branch 'upstream/master' into splitting-v3

6cee7c1

Merge remote-tracking branch 'upstream/master' into splitting-v3

9a576e0

# Conflicts: # protocol/src/main/java/org/geysermc/mcprotocollib/protocol/ClientListener.java # protocol/src/main/java/org/geysermc/mcprotocollib/protocol/ServerListener.java

Fix compilation

fc868b6

Revert a tiny bit closer to vanilla

59fcdf1

Camotoy reviewed Sep 19, 2024

View reviewed changes

AlexProgrammerDE added 3 commits September 19, 2024 20:44

Inline lambda

549170a

Inherit annotation

8f85079

Inherit annotation 2

f3d32cd

onebeastchris reviewed Sep 20, 2024

View reviewed changes

protocol/src/main/java/org/geysermc/mcprotocollib/network/tcp/TcpClientSession.java Outdated Show resolved Hide resolved

Use checkerframework annotation

ae1e7ed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split incoming/outgoing packet registry, transition protocol states correctly #841

Split incoming/outgoing packet registry, transition protocol states correctly #841

AlexProgrammerDE commented Jul 20, 2024 •

edited

Loading

AlexProgrammerDE commented Jul 26, 2024

Konicai left a comment

Konicai Jul 28, 2024

AlexProgrammerDE Jul 28, 2024

AlexProgrammerDE Jul 28, 2024

Konicai Aug 1, 2024

AlexProgrammerDE Aug 1, 2024

Konicai Aug 1, 2024

AlexProgrammerDE Aug 30, 2024

Konicai Sep 13, 2024

AlexProgrammerDE commented Sep 12, 2024

Konicai Sep 13, 2024

Konicai Sep 13, 2024

AlexProgrammerDE Sep 13, 2024

Camotoy Sep 19, 2024

Split incoming/outgoing packet registry, transition protocol states correctly #841

Are you sure you want to change the base?

Split incoming/outgoing packet registry, transition protocol states correctly #841

Conversation

AlexProgrammerDE commented Jul 20, 2024 • edited Loading

AlexProgrammerDE commented Jul 26, 2024

Konicai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexProgrammerDE commented Sep 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexProgrammerDE commented Jul 20, 2024 •

edited

Loading