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

Give Callers the Ability to set TCP_NODELAY #223

Closed
wants to merge 1 commit into from
Closed

Give Callers the Ability to set TCP_NODELAY #223

wants to merge 1 commit into from

Conversation

jcalabro
Copy link

Hey there! I've been using this library and having some performance issues for this specific application and network I'm on. I believe the issue may be due to the fact that TCP_NODELAY is not enabled.

I'm sure that I'm not the only one who may want this feature, which is enabled in node's http library, but not fetch (as far as I can tell)?

This PR also adds type annotations to the nodeHttpTransport and fixes up the associated linter errors that occurred as a result.

It is a bit unfortunate that it's asymmetric in nature (i.e. a caller could set noDelay: true, but not actually set it if they use the fetch RpcTransport). Thoughts on how to potentially improve that?

Thanks! Let me know if there are any questions I can answer!

@tatethurston
Copy link
Owner

Thanks @jcalabro, I’ll take a look. Just to make sure I’m understanding your current usage, you’re seeing degraded network performance for server to server communication?

@jcalabro
Copy link
Author

jcalabro commented Aug 30, 2023

I am having degraded performance on server -> server communication, yeah. Though I just ran a test and it turns out setting TCP_NODELAY made no significant difference in our particular workload.

That being said, I would still like to turn this on for all my RPCs. I typically write Go code, which has TCP_NODELAY on by default (some explanation here as to the reasoning), and I'd love to be able to do the same with my node twirp clients.

@tatethurston
Copy link
Owner

tatethurston commented Aug 30, 2023

@jcalabro Thanks for the PR. Identical to golang, TCP_NODELAY is on by default for nodejs

Closing because of this. LMK if you disagree.

@jcalabro
Copy link
Author

Oh interesting, thank you! Apologies, I didn't dig deep enough there. I do agree that we should close this PR. Thank you, apologies again!

@tatethurston
Copy link
Owner

No apologies necessary, I appreciate the MR.

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 this pull request may close these issues.

None yet

2 participants