Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue with client-desync in relation to server-ticking: #196

Open
PedroMPagani opened this issue Feb 14, 2024 · 10 comments
Open

Issue with client-desync in relation to server-ticking: #196

PedroMPagani opened this issue Feb 14, 2024 · 10 comments
Labels
status: needs triage type: bug Something doesn't work as it was intended to. version: 1.20.4 Game version 1.20.4

Comments

@PedroMPagani
Copy link

Expected behavior

The expected behavior is so that when the client sends packets, they get processed "immediately", currently what Folia does for every single packet is schedule each packet as a new task, for 1L. If you notice a simple debug, (I have done this) - The time between the packets arrive and get processed should be "instantly" - this is not really a vanilla only thing, but this affects PVP overall and other actions that players would notice a 50ms latency on their action, SPECIALLY if they have a low ping too.

Observed/Actual behavior

Tt really depends on how the player client ticking is also aligned and their ping, while on a paper jar, this won't happen because when the packet arrives it gets processed, the packets are delayed by one tick from when they arrive, and they don't get processed "instantly", they only get processed on the .executeTick() method, this is causing extra delays on actions overall, specially combat in survival.

Steps/models to reproduce

log System Millis of every single packet on the .genericsFtw, and on the ensure same thread that will make the runnable process, observe the behavior of when the .handle method gets called, this will show the latency issue.
I also noticed that the latency of processing of the packets, depends on how fast/ or the timing of in which the regions are created (I am not fully sure if this is because of region creation). - I am able to notice a difference upon restarting it though, sometimes less latency, sometimes high latency.

Plugin and Datapack List

No plugins nor data packets

Folia version

[12:28:35 INFO]: This server is running Folia version git-Folia-"8939611" (MC: 1.20.4) (Implementing API version 1.20.4-R0.1-SNAPSHOT) (Git: 8939611 on dev/1.20.4)
You are running the latest version
Previous version: git-Folia-"30ee81a" (MC: 1.20.2)

Other

No response

@PedroMPagani PedroMPagani added status: needs triage type: bug Something doesn't work as it was intended to. labels Feb 14, 2024
@PedroMPagani
Copy link
Author

I don't see lots of actual solutions for this, since the new threading is pretty much tasks scheduled and shared threads across regions, I feel like possibly having sub-ticks to process incoming packets on the same region scheduled threads would possibly help. (I implemented this).

@DrBotz
Copy link

DrBotz commented Feb 15, 2024

Yes

@Netherwhal
Copy link

This is one of the few issues we still face with Folia in comparison to Paper.

@kashike kashike added the version: 1.20.4 Game version 1.20.4 label Apr 29, 2024
@StateException
Copy link

I also noticed this. As I understand it, the logic is embedded in the ensureRunningOnSameThread code located in the PacketUtils class.

                gamePacketListener.player.getBukkitEntity().taskScheduler.schedule(
                    (net.minecraft.server.level.ServerPlayer player) -> {
                        run.run();
                    },
                    null, 1L
                );

Why do we need to call packet.handle after 1 tick? Can't we perform this operation immediately? Can someone explain the reason for this behavior? I would like to resolve this issue.

Thanks!

@electronicboy
Copy link
Member

electronicboy commented Jun 21, 2024

The earliest you can schedule a task on an entity is for the task to run the next time the entity ticks. You can't perform the operation immediately because you need to run the packet logic within the entities context.

The behavior here is going to be different because on a vanilla server where there is only 1 thread, you don't need to worry about dispatching packet handles into the correct thread context, there only is 1, and so you can just use the shared task queue which allows packets to be dispatched into the thread context which is heavily polled for work by the server.

@StateException
Copy link

The earliest you can schedule a task on an entity is for the task to run the next time the entity ticks. You can't perform the operation immediately because you need to run the packet logic within the entities context.

The behavior here is going to be different because on a vanilla server where there is only 1 thread, you don't need to worry about dispatching packet handles into the correct thread context, there only is 1, and so you can just use the shared task queue which allows packets to be dispatched into the thread context which is heavily polled for work by the server.

Thank you for the detailed explanation. I would like to clarify... So, logically, if we direct the packet immediately to the correct context before the ensureRunningOnSameThread check is called in ServerGamePacketListenerImpl.handle$Action, we will get the paper/vanilla behavior?

@electronicboy
Copy link
Member

The only current mechanism for ensuring that you get into that context is the entity scheduler, the thing is that the scheduler can only promise to run a task on the next tick. as regions do not spin wait between ticks, there is no mechanism to "immediately" post a task to run against an entity in a fast manner, and you have to remember that entities are not strictly held to a region, and so anything region based is also going to be precarious

@Luuzzi
Copy link

Luuzzi commented Jun 23, 2024

The only current mechanism for ensuring that you get into that context is the entity scheduler, the thing is that the scheduler can only promise to run a task on the next tick. as regions do not spin wait between ticks, there is no mechanism to "immediately" post a task to run against an entity in a fast manner, and you have to remember that entities are not strictly held to a region, and so anything region based is also going to be precarious

Thank you for the clear response. Purely hypothetically, we can’t divide these 50ms into subticks (for example, every 1ms) and perform only “urgent” operations in them? Won't this lead to some unexpected behavior or something?

UPD: I understand that there is no such mechanism now, I’m just wondering.

@PedroMPagani
Copy link
Author

Yes. That's what's being done on DonutSMP. (Not 1ms though, since that's not very CPU-wise and you'll likely need low latency kernels for that as well usually). 200 TPS. 5ms subticks.

@Luuzzi
Copy link

Luuzzi commented Jun 24, 2024

Yes. That's what's being done on DonutSMP. (Not 1ms though, since that's not very CPU-wise and you'll likely need low latency kernels for that as well usually). 200 TPS. 5ms subticks.

Good solution, glad to hear it works, btw thanks for info.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: needs triage type: bug Something doesn't work as it was intended to. version: 1.20.4 Game version 1.20.4
Projects
None yet
Development

No branches or pull requests

7 participants