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

Consider adding explicit capability to set TCP_NODELAY on client #1262

Closed
abls opened this issue Dec 10, 2023 · 1 comment · Fixed by #1263
Closed

Consider adding explicit capability to set TCP_NODELAY on client #1262

abls opened this issue Dec 10, 2023 · 1 comment · Fixed by #1263

Comments

@abls
Copy link

abls commented Dec 10, 2023

Currently in order to set TCP_NODELAY on a socket used by an HttpClient, one can use (abuse?) the ability to set middleware in the HttpClientBuilder like this:

let client_builder =
    HttpClientBuilder::default().set_middleware(tower::ServiceBuilder::new().layer_fn(|_| {
        let mut connector = HttpConnector::new();
        connector.set_nodelay(true);

        // TODO handle https? copy line 120-135 in client/http-client/src/transport.rs
        HttpBackend::Http(Client::builder().build(connector))
    }));

let client = client_builder.build(url).unwrap();

This feels kind of like a hack since I must override the HttpBackend and essentially render this whole block of code useless. Also not shown in the example snippet above is the necessary logic to construct an HttpBackend::Https if the url I plan to connect to is https, which breaks the expectation that client_builder.build(url) works for either http or https since I now must know the url in advance.

All of this complexity is required to set an option that perhaps should be set by default in an RPC library. At least, there should be a more ergonomic way to set this, so users don't have to reimplement logic internal to the library while also breaking expected behavior.

Without TCP_NODELAY, I can observe latency on a request to localhost stepping up from 1ms to 40ms after adding one byte to the request body beyond a certain threshold. In latency-sensitive use-cases, this matters a lot and it is necessary for users to be able to set TCP_NODELAY on the client.

Is there a better way to set this option that I have missed? Is the way I have found to set it through overriding the HttpBackend service through middleware even something that was intended? If there was no intended way to set this, are you open to adding a more ergonomic way to do so? Do you have any strong opinions about setting TCP_NODELAY to true by default on the client, or perhaps disabling delayed ACKs by default on the server?

@niklasad1
Copy link
Member

niklasad1 commented Dec 10, 2023

Currently in order to set TCP_NODELAY on a socket used by an HttpClient, one can use (abuse?) the ability to set middleware in the HttpClientBuilder like this:

Agree, this is bad

Is there a better way to set this option that I have missed? Is the way I have found to set it through overriding the HttpBackend service through middleware even something that was intended? If there was no intended way to set this, are you open to adding a more ergonomic way to do so? Do you have any strong opinions about setting TCP_NODELAY to true by default on the client, or perhaps disabling delayed ACKs by default on the server?

No, let's make TCP_NODELAY to true by default and add a builder option to opt-out if someone wants that.
We have that enabled TCP_NODELAY == true in the servers and it makes sense here as well, simply missed I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants