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

rustyconover · 2020-01-27T16:23:18Z

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

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

sam-github · 2020-01-27T16:49:27Z

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 · 2020-01-27T17:05:36Z

@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 · 2020-01-27T17:15:56Z

re. TCP_NODELAY I don't think we ever did implement #906.

sam-github · 2020-01-27T17:24:59Z

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 · 2020-01-27T17:30:03Z

@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 · 2020-01-27T17:38:29Z

@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 · 2020-01-27T17:51:33Z

@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 (#7995) but it stalled out.

jasnell · 2020-01-27T17:57:15Z

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 · 2020-01-28T00:39:24Z

This is greeeat! It would be nice to cherry pick the stuff from #31541

addaleax

I’m not sure how I feel about this – if you want to set socket options manually, wouldn’t createConnection() be a more sensible place to do this (as this is more concerned with networking and not really with HTTP itself)?

doc/api/http.md

addaleax

After checking, it looks like the net module does not provide an option like this – I think that if we add this to HTTP, we should definitely also add it to net.Socket. But again, it feels to me like this is better kept on the networking side only.

rustyconover · 2020-01-28T14:27:35Z

@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 · 2020-01-28T16:35:33Z

@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.

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

Add the noDelay option to the net.Socket() options such that .setNoDelay() will be called when a socket is created

rustyconover · 2020-02-26T12:27:07Z

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

rustyconover · 2020-03-01T15:09:02Z

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

ronag

I think this should have some tests.

voxpelli · 2020-07-03T16:54:45Z

@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: #34185

mcollina

I think we might want to set this also for tls.

Moreover, we need tests.

rustyconover · 2020-08-11T03:38:34Z

@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?

rickyes · 2020-08-11T04:24:49Z

doc/api/net.md

@@ -417,6 +417,9 @@ added: v0.3.4
  * `allowHalfOpen` {boolean} Indicates whether half-opened TCP connections
    are allowed. See [`net.createServer()`][] and the [`'end'`][] event
    for details. **Default:** `false`.
+  * `noDelay` {boolean} A boolean with which to call [`socket.setNoDelay()`][]
+    when a socket connection is established. This only applies when the
+    underlying connection is a TCP socket.


Please add default values.

mcollina · 2020-08-11T06:29:49Z

Do you have an idea of an adequate test plan?

I would just override the setNoDelay method.

voxpelli · 2020-10-14T08:44:44Z

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 · 2020-10-14T14:53:29Z

That's good enough as far as tests goes?

I think so, yes.

aduh95 · 2020-11-17T15:28:31Z

@rustyconover Do you plan to keep working on this?

github-actions · 2020-11-17T15:28:56Z

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 · 2020-11-17T16:54:03Z

Sure, I think I'll keep at it.

voxpelli · 2020-11-17T17:17:50Z

@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 · 2021-06-05T10:41:47Z

Stalled again.

mcollina · 2021-06-05T15:22:42Z

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

bl-ue · 2021-06-05T16:42:56Z

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 · 2021-06-05T18:18:22Z

@rustyconover are you still interested in pursuing this?

rustyconover · 2021-06-05T19:39:07Z

I'm still interested in landing this...

voxpelli · 2021-09-15T17:10:48Z

This isn't a breaking change, right? So it can go out in any minor? So not important to eg. get it into #40119 ?

mcollina · 2021-09-15T17:39:57Z

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

aduh95

Can you please add YAML entries similar to

node/doc/api/net.md

Lines 545 to 548 in 6fdd582

    
           changes: 
        
             - version: v15.14.0 
        
               pr-url: https://github.com/nodejs/node/pull/37735 
        
               description: AbortSignal support was added.

nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Jan 27, 2020

jasnell approved these changes Jan 27, 2020

View reviewed changes

rustyconover mentioned this pull request Jan 27, 2020

docs: Reword socket.setNoDelay() #31541

Closed

3 tasks

rustyconover mentioned this pull request Jan 28, 2020

net: Track state of setNoDelay and prevent unnecessary system calls. #31543

Closed

4 tasks

addaleax reviewed Jan 28, 2020

View reviewed changes

doc/api/http.md Outdated Show resolved Hide resolved

addaleax requested changes Jan 28, 2020

View reviewed changes

rustyconover force-pushed the fix-no-delay-http-options branch from cf9e4ef to 35ace28 Compare January 28, 2020 14:29

rustyconover added 2 commits February 26, 2020 07:14

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

844418c

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

net: Add noDelay to net.Socket() options

24697c2

Add the noDelay option to the net.Socket() options such that .setNoDelay() will be called when a socket is created

rustyconover force-pushed the fix-no-delay-http-options branch from 35ace28 to 24697c2 Compare February 26, 2020 12:14

rustyconover requested a review from addaleax February 26, 2020 12:26

ronag requested changes Mar 11, 2020

View reviewed changes

BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:19

voxpelli mentioned this pull request Jul 3, 2020

Follow curl: Enable setNoDelay() / TCP_NODELAY by default on HTTP(S) 1.x #34185

Closed

rustyconover requested a review from a team as a code owner August 10, 2020 16:07

rustyconover requested a review from a team August 10, 2020 16:07

mcollina reviewed Aug 10, 2020

View reviewed changes

rickyes reviewed Aug 11, 2020

View reviewed changes

aduh95 added the stalled Issues and PRs that are stalled. label Nov 17, 2020

aduh95 removed the stalled Issues and PRs that are stalled. label Nov 17, 2020

guybedford force-pushed the master branch from dc5a5da to 8e46568 Compare March 29, 2021 21:33

aduh95 added the semver-minor PRs that contain new features and should be released in the next minor version. label Sep 15, 2021

aduh95 reviewed Sep 15, 2021

View reviewed changes

voxpelli mentioned this pull request Nov 16, 2021

net: activate setNoDelay by default #40778

Closed

3 tasks

Trott force-pushed the main branch from 2d76238 to ca3ed36 Compare November 12, 2022 01:49

	changes:
	- version: v15.14.0
	pr-url: https://github.com/nodejs/node/pull/37735
	description: AbortSignal support was added.

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

Are you sure you want to change the base?

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

Conversation

rustyconover commented Jan 27, 2020

sam-github commented Jan 27, 2020

rustyconover commented Jan 27, 2020 • edited Loading

richardlau commented Jan 27, 2020

sam-github commented Jan 27, 2020

rustyconover commented Jan 27, 2020

rustyconover commented Jan 27, 2020

richardlau commented Jan 27, 2020

jasnell commented Jan 27, 2020

juanarbol commented Jan 28, 2020

addaleax left a comment

Choose a reason for hiding this comment

addaleax left a comment

Choose a reason for hiding this comment

rustyconover commented Jan 28, 2020

addaleax commented Jan 28, 2020

rustyconover commented Feb 26, 2020

rustyconover commented Mar 1, 2020

ronag left a comment

Choose a reason for hiding this comment

voxpelli commented Jul 3, 2020

mcollina left a comment

Choose a reason for hiding this comment

rustyconover commented Aug 11, 2020

rickyes Aug 11, 2020

Choose a reason for hiding this comment

mcollina commented Aug 11, 2020

voxpelli commented Oct 14, 2020

mcollina commented Oct 14, 2020

aduh95 commented Nov 17, 2020

github-actions bot commented Nov 17, 2020

rustyconover commented Nov 17, 2020

voxpelli commented Nov 17, 2020

bl-ue commented Jun 5, 2021

mcollina commented Jun 5, 2021

bl-ue commented Jun 5, 2021

mcollina commented Jun 5, 2021

rustyconover commented Jun 5, 2021

voxpelli commented Sep 15, 2021

mcollina commented Sep 15, 2021

aduh95 left a comment

Choose a reason for hiding this comment

rustyconover commented Jan 27, 2020 •

edited

Loading