tatethurston / TwirpScript

A protobuf RPC framework for JavaScript and TypeScript
MIT License
141 stars 14 forks source link

Give Callers the Ability to set TCP_NODELAY #223

Closed jcalabro closed 1 year ago

jcalabro commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

No apologies necessary, I appreciate the MR.