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

http: make TCP noDelay enabled by default #42163

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 8 additions & 1 deletion doc/api/http.md
Expand Up @@ -2838,6 +2838,13 @@ Found'`.
<!-- YAML
added: v0.1.13
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/42163
description: The `noDelay` option now defaults to `true`.
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/41310
description: The `noDelay`, `keepAlive` and `keepAliveInitialDelay`
options are supported now.
Comment on lines +2846 to +2847
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this PR only changes noDelay now? (Same with the change to net.md).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically yes. But in the PR where I originally added it I forgot to update the docs' changelog. 😞

Shall I revert the modification or is it good?

Copy link
Member

@richardlau richardlau Mar 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I didn't spot the PR-URL was for an earlier PR. Keep those, but add a new entry for this PR noting the default has been changed for noDelay.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! Does it look fine now?

- version:
- v13.8.0
- v12.15.0
Expand Down Expand Up @@ -2871,7 +2878,7 @@ changes:
**Default:** 16384 (16 KB).
* `noDelay` {boolean} If set to `true`, it disables the use of Nagle's
algorithm immediately after a new incoming connection is received.
**Default:** `false`.
**Default:** `true`.
* `keepAlive` {boolean} If set to `true`, it enables keep-alive functionality
on the socket immediately after a new incoming connection is received,
similarly on what is done in \[`socket.setKeepAlive([enable][, initialDelay])`]\[`socket.setKeepAlive(enable, initialDelay)`].
Expand Down
4 changes: 4 additions & 0 deletions doc/api/net.md
Expand Up @@ -822,6 +822,10 @@ behavior.
<!-- YAML
added: v0.1.90
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/41310
description: The `noDelay`, `keepAlive` and `keepAliveInitialDelay`
options are supported now.
Comment on lines +825 to +828
Copy link
Contributor

@aduh95 aduh95 Mar 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should have gone to its own PR, since this PR is semver-major it will land on v18.0.0, while #41310 will land on v17.7.0.

EDIT: PR to fix that is #42268.

- version: v12.10.0
pr-url: https://github.com/nodejs/node/pull/25436
description: Added `onread` option.
Expand Down
3 changes: 3 additions & 0 deletions lib/_http_agent.js
Expand Up @@ -101,6 +101,9 @@ function Agent(options) {

this.options = { __proto__: null, ...options };

if (this.options.noDelay === undefined)
this.options.noDelay = true;

// Don't confuse net and make it think that we're connecting to a pipe
this.options.path = null;
this.requests = ObjectCreate(null);
Expand Down
3 changes: 3 additions & 0 deletions lib/_http_server.js
Expand Up @@ -363,6 +363,9 @@ function storeHTTPOptions(options) {
if (insecureHTTPParser !== undefined)
validateBoolean(insecureHTTPParser, 'options.insecureHTTPParser');
this.insecureHTTPParser = insecureHTTPParser;

if (options.noDelay === undefined)
options.noDelay = true;
}

function Server(options, requestListener) {
Expand Down
37 changes: 37 additions & 0 deletions test/parallel/test-http-nodelay.js
@@ -0,0 +1,37 @@
'use strict';

const common = require('../common');
const assert = require('assert');
const http = require('http');
const net = require('net');

const originalConnect = net.Socket.prototype.connect;

net.Socket.prototype.connect = common.mustCall(function(args) {
assert.strictEqual(args[0].noDelay, true);
return originalConnect.call(this, args);
});

const server = http.createServer(common.mustCall((req, res) => {
res.writeHead(200);
res.end();
server.close();
}));

server.listen(0, common.mustCall(() => {
assert.strictEqual(server.noDelay, true);

const req = http.request({
method: 'GET',
port: server.address().port
}, common.mustCall((res) => {
res.on('end', () => {
server.close();
res.req.socket.end();
});

res.resume();
}));

req.end();
}));