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

[WebSocket] Ensure TCP_NODELAY is always set #94618

Merged
merged 1 commit into from
Jul 22, 2024

Conversation

Faless
Copy link
Collaborator

@Faless Faless commented Jul 22, 2024

Almost all WebSocket implementations (including all major browsers) disable Nagle's algorithm to favor low latency over packet overhead.

This was also the case in Godot 3.x, while in Godot 4.0 this was only being done for clients and wasn't even always working due to a bug.

This commit fixes the aforementioned bug, and forces TCP_NODELAY when accepting a stream as a server.

Fixes #86234

@Faless Faless added topic:network regression cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Jul 22, 2024
@Faless Faless added this to the 4.3 milestone Jul 22, 2024
@Faless Faless requested a review from a team as a code owner July 22, 2024 12:20
@Faless
Copy link
Collaborator Author

Faless commented Jul 22, 2024

Unlike what mentioned in #86234 (comment) , I decided always set the no delay option.

That was the original intent, and is in line with most implementations:

https://bugzilla.mozilla.org/show_bug.cgi?id=542401

https://issues.chromium.org/issues/41152920

https://bugs.webkit.org/show_bug.cgi?id=102079

It's worth noting that languages like go, defaults to disable the Nagle's algorithm even if most OSes would otherwise have different default (see here).

We should evaluate if we want to do something similar in Godot's socket implementation (in its own proposal/PR).

@akien-mga akien-mga changed the title [WebSocket] Ensure TCP_NODELAY is always set [WebSocket] Ensure TCP_NODELAY is always set Jul 22, 2024
@akien-mga
Copy link
Member

So IIUC this fixes the pre-existing docs for WebSocketPeer? As they mention "no delay" is the default.

                <method name="set_no_delay">
                        <return type="void" />
                        <param index="0" name="enabled" type="bool" />
                        <description>
                                Disable Nagle's algorithm on the underling TCP socket (default). See [method StreamPeerTCP.set_no_delay] for more information.
                                [b]Note:[/b] Not available in the Web export.
                        </description>
                </method>

BTW I notice a typo in "underling" (should be "underlying"), worth fixing in this PR while at it.

@akien-mga akien-mga added bug and removed cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Jul 22, 2024
Almost all WebSocket implementations (including all major browsers)
disable Nagle's algorithm to favor low latency over packet overhead.

This was also the case in Godot 3.x, while in Godot 4.0 this was only
being done for clients and wasn't even always working due to a bug.

This commit fixes the aforementioned bug, and forces TCP_NODELAY when
accepting a stream as a server.
@Faless Faless force-pushed the ws/fix_no_delay branch from 786f094 to d65e7aa Compare July 22, 2024 12:41
@Faless
Copy link
Collaborator Author

Faless commented Jul 22, 2024

So IIUC this fixes the pre-existing docs for WebSocketPeer? As they mention "no delay" is the default.

Indeed, I didn't notice it was even in the documentation.

BTW I notice a typo in "underling" (should be "underlying"), worth fixing in this PR while at it.

I've added the fix for the typo.

@akien-mga akien-mga merged commit 9d8c2c9 into godotengine:master Jul 22, 2024
18 checks passed
@akien-mga
Copy link
Member

Thanks!

@Faless Faless deleted the ws/fix_no_delay branch July 23, 2024 03:08
@akien-mga akien-mga removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Mar 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release regression topic:network
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TCP_NODELAY is disabled by default in WebSocket communication in 4.x, but was enabled by default in 3.x
2 participants