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

Feature: Serve::tcp_nodelay #2653

Merged
merged 5 commits into from Apr 1, 2024

Conversation

Clueliss
Copy link
Contributor

Fixes: #2521

Motivation

See #2521 and linked issue in hyper

Solution

Adds the tcp_nodelay method to Serve (and WithGracefulShutdown) to enable the user to instruct the server to set TCP_NODELAY on incoming connections.

Regarding tests: I wasn't really sure what you would like to have there (since with_graceful_shutdown also doesn't seem to have tests or I didn't find them, not sure), so pointers are appreciated.

@mladedav
Copy link
Contributor

Hi, this looks good to me. Can you just add an entry to the changelog please?

Copy link
Member

@jplatte jplatte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is TCP_NODELAY ever on by default, such that .tcp_nodelay(false) would make sense to have? I was originally thinking that the method would be parameter-less.


/// Instructs the server to set the value of the `TCP_NODELAY` option on every accepted connection.
///
/// See also [`TcpStream::set_nodelay`]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// See also [`TcpStream::set_nodelay`]
/// See also [`TcpStream::set_nodelay`].

impl<M, S, F> WithGracefulShutdown<M, S, F> {
/// Instructs the server to set the value of the `TCP_NODELAY` option on every accepted connection.
///
/// See also [`TcpStream::set_nodelay`]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// See also [`TcpStream::set_nodelay`]
/// See also [`TcpStream::set_nodelay`].

@Clueliss
Copy link
Contributor Author

Is TCP_NODELAY ever on by default, such that .tcp_nodelay(false) would make sense to have? I was originally thinking that the method would be parameter-less.

The idea was that there is maybe some configuration or OS where it could be enabled by default that I'm just not aware of. Just wanted to make sure that we don't mess with that default value and users can explicitly disable it.

Again, I personally couldn't give you an example of such an OS or config.
I just had hyperium/hyper#3269 (comment) in mind where it was mentioned that MacOS probably has a slightly different implementation.

("[..] On Mac OS there was no difference in performance between /stream and /regular and this probably has to do with a slight difference in the implementation details or settings tuning of Nagle's algorithm across the two OSes. [..]" - @huntharo)

The comment doesn't imply that its not enabled tho.
I can make it parameter-less if you prefer

@Clueliss Clueliss requested a review from jplatte March 20, 2024 07:27
@jplatte
Copy link
Member

jplatte commented Mar 20, 2024

Hey, I'll try to get to this again soon. Your reasoning for having the bool parameter makes sense, let's keep that.

Copy link
Member

@jplatte jplatte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, could you rebase on top of / merge main? Should fix CI and merge conflicts.

@jplatte jplatte added the S-waiting-on-author Status: awaiting some action (such as code changes) from the PR or issue author. label Apr 1, 2024
@jplatte jplatte merged commit 50c035c into tokio-rs:main Apr 1, 2024
18 checks passed
ttys3 added a commit to ttys3/static-server that referenced this pull request Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: awaiting some action (such as code changes) from the PR or issue author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set tcp nodelay on sockets
3 participants