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

[voxpelli]

Prior art

Issues

This has been discussed before without any conclusion:

• 2015-2018 in net: enable TCP_NODELAY by default on all platforms #906
• 2015 in Default nodelay value needs to be set for all sockets.  node-v0.x-archive#9235

Relatedly #906 and the discussion there has been mentioned:

• Recently, in the beginning of 2020, in http: Add noDelay to http.request() options. #31539 as well as docs: Reword socket.setNoDelay() #31541

Commits

The setNoDelay() option was originally introduced in 2009 in e0ec003 and was shipped in v0.1.12

Wider context

curl

In 2016 curl changed it's behavior from being like the one in Node.js to instead be setting TCP_NODELAY by default. See curl/curl@4732ca5 which was released in v7.60.2.

Motivation from commit:

After a few wasted hours hunting down the reason for slowness during a TLS handshake that turned out to be because of TCP_NODELAY not being set, I think we have enough motivation to toggle the default for this option. We now enable TCP_NODELAY by default and allow applications to switch it off.

npm modules

agentkeepalive

The agentkeepalive module, with ≈3.5 million weekly downloads, pushes its non-configurable default of socket.setNoDelay(true) as one of its major benefits.

It was added in node-modules/agentkeepalive@c92f5b5 and references a Scaling node.js to 100k concurrent connections! blog post to explain it.

Used by eg. eggjs/egg, cnpm/cnpmjs.org, pubnub/javascript, googlemaps/google-maps-services-js, node-modules/urllib

superagent

In 2017 the superagent module, also with ≈3.5 million weekly downloads, merged ladjs/superagent#1240 and made socket.setNoDelay(true) its non-configurable default, largely arguing that there was no need for any buffering.

Other npm modules

Many modules relies on the default behaviour of Node.js, and thus does not disable any delay by default.

A quick glance puts all of these in that category, none of them directly calls setNoDelay (though they could eg. use something like the agentkeepalive module): axios, bent, got, node-fetch, request, unfetch

Documentation

The Node.js documentation of setNoDelay was recently updated: #31541

Before that update, my perception is that many believed that Nagle's algorithm wasn't enabled by defaults, as proved by eg. the conversation in superagent: ladjs/superagent#1240

Proposed change

To make TCP_NODELAY / setNoDelay the default for HTTP(S) 1.x + maybe introduce a setDelay() method to disable it.

Probably in a new major version, as it can be a breaking change in some contexts.

Why

I believe that the expectations has changed since setNoDelay was introduced in 2009.

With eg. curl now setting it by default I believe most expects TCP_NODELAY / setNoDelay to be set by default and thus Nagle's algorithm to be disabled by default.

The confusion the current default seems to make in issues like ladjs/superagent#1240, especially before #31541, supports that.

Also: As TCP_NODELAY / Nagle's algorithm is implemented at an OS level this can also have a minor added benefit of improved cross-platform behavior, as no platform will add a delay.

Benefits

• Better comply with general expectations among developers
• Move with the times and in sync with other major players like curl
• In some cases better performance due to no delay. Especially so in real time scenarios.

Downsides

• Possibly increased bandwidth use
• Possibly decreased performance due to bandwidth saturation
• As any change of a default on this level, it breaks the expectations of those who knowingly wants the current default behaviour.

My background

I have no expertise in the implementation of Nagle's algorithm or the TCP-stack in general, though I became intrigued by this and did my research and through that research I found that most were either confused by this or had already changed the default for this.

Comments, suggestions, feedback and link to other discussions would be most helpful.

[voxpelli]

Clarified that my focus here was on HTTP, not other kinds of TCP communication

[jasnell]

So, going back and reading through the history a bit, it seems that @bnoordhuis had some concerns about core's ability to batch writes effectively enough for the NO_DELAY to be effective (or even functional). @bnoordhuis, do you still have the same concerns?

[rustyconover]

@voxpelli Thank you for putting this issue together, as a person who's been pointing at the NO_DELAY windmills for a while I completely understand your desire to see this resolved.

Sometimes doing the right thing is just hard. Setting NO_DELAY seems to be the right thing especially now that overall network bandwidths have increased and especially in datacenter to datacenter communications.

I think #31539 is ready to go, @jasnell's request for tests is still outstanding but I've been doing other things. Hopefully this issue could move things more towards #31539 being merged.

[bnoordhuis]

I believe the I/O layer is good enough nowadays to turn on nodelay. Any performance regressions - e.g., because HTTP headers and request body now get split across TCP packets - should be simple fixes.

[voxpelli]

Would it make sense to target this for Node 16? I guess it's too late for Node 15, right @jasnell @bnoordhuis?

[silverwind]

I support this. Is there a specific reason why HTTP 2 is not mentioned?

[voxpelli]

@silverwind I'm not sure HTTP2 has the same default, and I believe it's a different implementation to HTTP1 so would probably be a follow-up fix, right @jasnell? HTTP3 / QUIC shouldn't be affected as all as it doesn't use TCP?

[voxpelli]

Would it be possible to get this into 17.0.0? #40119

Or should we leave it as is pending the future of the Node HTTP client? #38533

Considering that Undici seems to be having this set already: https://github.com/nodejs/undici/blob/11c2db1672c55acc58e7813a4aec579ad644dfc7/lib/core/connect.js#L74-L75

Thoughts @mcollina?

[mcollina]

I support this change. I'm not even sure it needs to be semver-major minus prudence (we shipped #36685 as minor with similar perf benefits).

I would just recommend we add an option to the Agent to re-enable it if they want to.

[mcollina]

@voxpelli do you think you could open a PR for this?

[voxpelli]

@mcollina I can give it a go, about time that I do a PR to core

[voxpelli]

@mcollina I made an initial stab almost 2 months ago but didn't get further with tests and such. I opened a PR with what I have and maybe we can try and make it progress collaboratively: #40778