http: Add noDelay to http.request() options.

[rustyconover]

Add the noDelay option to the http.request() options such that .setNoDelay() will be called once a socket is obtained.

• [x] make -j4 test (UNIX), or vcbuild test (Windows) passes
• [x] documentation is changed or added
• [x] commit message follows commit guidelines

[sam-github]

Hi, what is the purpose of this?

Default is no delay is true (so nagle is off).

Is there any time you would want nagle on for HTTP? I can't think of one.

Is there any way that http.request() would somehow get a net Socket that had nagle turned on still? I can't think of one, but I might be missing some corner case.

[rustyconover]

@sam-github Hi, I'm sure your tone is nice and friendly but others might interpret it as a bit aggressive and unwelcoming asking what the purpose is. I think the patch is quite clear in its purpose.

By default tcp sockets are created with the Nagle's delay algorithm enabled. See https://en.wikipedia.org/wiki/Nagle%27s_algorithm. Would you be kind enough to show where you think the default disabling of TCP_NODELAY being set?

That call isn't happening in the strace output that I have of my simple test cases making a request via both HTTPS and HTTP in against node HEAD.

[richardlau]

re. TCP_NODELAY I don't think we ever did implement https://github.com/nodejs/node/issues/906.

[sam-github]

My apologies @rustyconover, I don't mean to discourage you. Your description was quite clear in the what, but if you look at it again, you will notice that it does not say anything about why, and it is difficult to review a PR without understanding what goal it achieves.

Would you be kind enough to show where you think the default disabling of TCP_NODELAY being set?

My assumption was the documentation was correct, though given the conversation @richardlau linked to, it seems it is not. :-(

• https://nodejs.org/api/net.html#net_socket_setnodelay_nodelay

[rustyconover]

@sam-github It is all good.

My goal is to allow people to control disabling the delay in HTTP options rather than forcing users to call .setNoDelay() on each connection after the fact. It is quite difficult in some code that only takes high-level HTTP options (such as the AWS SDK) to get them to surface the actual request objects that are created so I can disable the Nagle algorithm. Making noDelay available on the HTTP options lets me achieve this goal with the minimum of fussing around.

[rustyconover]

@sam-github Also the docs imply the default for the parameter to .setNoDelay() is true if it is not passed, it says nothing about the default for the underlying socket connection when it is created. Should this be made more clear so others don't make the same mistake?

[richardlau]

@sam-github Also the docs imply the default for the parameter to .setNoDelay() is true if it is not passed, it says nothing about the default for the underlying socket connection when it is created. Should this be made more clear so others don't make the same mistake?

I think so. There was a previous attempt at this (https://github.com/nodejs/node/pull/7995) but it stalled out.

[jasnell]

While I'm ok with this in general, I think we should (as a separate activity) explore a better model for passing options down to internal dependent components in general. We have this exact kind of problem (passing options down) in multiple areas and we end up dealing with it in one off ways every time.

[juanarbol]

This is greeeat! It would be nice to cherry pick the stuff from https://github.com/nodejs/node/pull/31541

[rustyconover]

@addaleax Hi,

I'm happy to add the same functionality to new net.Socket([options]) in a different PR if you think that's reasonable.

I believe having the noDelay option at the HTTP level is ok right now. The reason is a lot of popular high level packages don't expose the underlying network sockets they use to the user but they typically do expose HTTP options. By not having the functionality here to disable Nagle's algorithm, you're asking every user to pass a custom HTTP Agent that would then hook the createConnection() method and call .setNoDelay() on the socket itself. That seems like too much repetitive burden for something that can be accomplished here for them.

By default all TCP connections have Nagle's algorithm enabled, which is less than optimal for HTTP/HTTPS. Until that changes (i.e. Node decides to disable it by default for HTTP/HTTPS) and it seems like this is the best way forward.

I'm interested to know what you think.

[addaleax]

@rustyconover To be clear, I’m not opposed to having an option like this in HTTP.

However, I’d prefer to have this in net first in order not to create a situation where we have a feature in HTTP that should be implemented in terms of the net option (thus removing orthogonality here).

I’m okay with this waiting for a PR for net first, but as far as I’m concerned you can just add a separate commit to this one.

[rustyconover]

@addaleax I've added the options to net.Socket() as you've requested in a second commit.

[rustyconover]

@jasnell can you please add the author-ready tag?

[voxpelli]

@sam-github @richardlau I have posted a new PR that replaces the closed #906 and again pushes for changing the default behavior to be TCP_NODELAY: https://github.com/nodejs/node/issues/34185

[rustyconover]

@mcollina I think tests for this feature run the risk of not showing anything interesting as the TCP_DELAY behavior isn't super deterministic between operating systems or even versions of the same operating system.

Do you have an idea of an adequate test plan?

[mcollina]

Do you have an idea of an adequate test plan?

I would just override the setNoDelay method.

[voxpelli]

Do you have an idea of an adequate test plan?

I would just override the setNoDelay method.

So basically:

Override the setNoDelay method in two tests – one that ensures it's called when the option is set and one that ensures it isn't when the option isn't set.

That's good enough as far as tests goes?

[mcollina]

That's good enough as far as tests goes?

I think so, yes.

[aduh95]

@rustyconover Do you plan to keep working on this?

[github-actions[bot]]

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

[rustyconover]

Sure, I think I'll keep at it.

[voxpelli]

@rustyconover Feels like the tests should be fairly quick to wrap up, need some help or you have time to fix them yourself?

[bl-ue]

Stalled again.

[mcollina]

would you like to pick it up @bl-ue?

[bl-ue]

Ah @mcollina, I feel honored by your offer 😁 ❤️, but I'm afraid I've neither the time nor the expertise in this area to continue it :/

[mcollina]

@rustyconover are you still interested in pursuing this?

[rustyconover]

I'm still interested in landing this...

[voxpelli]

This isn't a breaking change, right? So it can go out in any minor? So not important to eg. get it into https://github.com/nodejs/node/pull/40119 ?

[mcollina]

I prefer the option being on the Agent and not on the http.request().