http: provide keep-alive timeout response header by ronag · Pull Request #34561 · nodejs/node

ronag · 2020-07-30T14:59:56Z

In http 1.1 persistent connection protocol there is a timing race where the
client sends the request and then the server kills the connection
(due to inactivity) before receiving the client's request.

By providing a keep-alive header it is possible to provide the client a
hint of when idle timeout would occur and avoid the race.

Fixes: #34560

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

ronag · 2020-07-30T15:00:05Z

@nodejs/http

In http 1.1 persistent connection protocol there is a timing race where the client sends the request and then the server kills the connection (due to inactivity) before receiving the client's request. By providing a keep-alive header it is possible to provide the client a hint of when idle timeout would occur and avoid the race. Fixes: nodejs#34560

ronag · 2020-07-30T15:28:52Z

lib/_http_outgoing.js

    if (shouldSendKeepAlive) {
      header += 'Connection: keep-alive\r\n';
+      if (this._keepAliveTimeout) {
+        const timeoutSeconds = MathFloor(this._keepAliveTimeout) / 1000;


Should we always set this to less than the actual timeout? And if so what is a good algorithm?

Based nodejs/undici#279 I feel that any client should always assume a value less than the hint. So further reducing it here can be counter productive.

ronag · 2020-07-30T15:58:09Z

@nodejs/http @nodejs/web-server-frameworks

nodejs-github-bot · 2020-07-30T15:58:55Z

CI: https://ci.nodejs.org/job/node-test-pull-request/32556/

nodejs-github-bot · 2020-07-30T16:09:36Z

CI: https://ci.nodejs.org/job/node-test-pull-request/32558/

mcollina

lgtm, it would be good if our own agent would do somethign about this as well.

ronag · 2020-08-01T13:05:23Z

Landed in 849d9e7

In http 1.1 persistent connection protocol there is a timing race where the client sends the request and then the server kills the connection (due to inactivity) before receiving the client's request. By providing a keep-alive header it is possible to provide the client a hint of when idle timeout would occur and avoid the race. Fixes: #34560 PR-URL: #34561 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Zeyu Yang <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Pranshu Srivastava <[email protected]>

killagu · 2020-09-07T05:27:57Z

lib/_http_outgoing.js

      header += 'Connection: keep-alive\r\n';
+      if (this._keepAliveTimeout) {
+        const timeoutSeconds = MathFloor(this._keepAliveTimeout) / 1000;
+        header += `Keep-Alive: timeout=${timeoutSeconds}\r\n`;


Not add the header if the Keep-Alive exits maybe better.

PR-URL: #35138 Fixes: #34561 Reviewed-By: Ricky Zhou <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Zeyu Yang <[email protected]>

In http 1.1 persistent connection protocol there is a timing race where the client sends the request and then the server kills the connection (due to inactivity) before receiving the client's request. By providing a keep-alive header it is possible to provide the client a hint of when idle timeout would occur and avoid the race. Fixes: #34560 PR-URL: #34561 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Zeyu Yang <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Pranshu Srivastava <[email protected]>

Backport-PR-URL: #35623 PR-URL: #35138 Fixes: #34561 Reviewed-By: Ricky Zhou <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Zeyu Yang <[email protected]>

PR-URL: nodejs#35138 Fixes: nodejs#34561 Reviewed-By: Ricky Zhou <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Zeyu Yang <[email protected]>

ronag requested a review from mcollina July 30, 2020 14:59

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

ronag force-pushed the keep-alive-hint branch from d2c88ca to 9719ec1 Compare July 30, 2020 15:00

addaleax approved these changes Jul 30, 2020

View reviewed changes

fixup

003e581

ronag force-pushed the keep-alive-hint branch from 686f8f2 to 003e581 Compare July 30, 2020 15:06

ronag commented Jul 30, 2020

View reviewed changes

fixup

b4e8b79

ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 30, 2020

mcollina approved these changes Jul 30, 2020

View reviewed changes

lpinca approved these changes Jul 30, 2020

View reviewed changes

himself65 approved these changes Jul 30, 2020

View reviewed changes

rickyes approved these changes Jul 31, 2020

View reviewed changes

trivikr approved these changes Jul 31, 2020

View reviewed changes

rexagod approved these changes Jul 31, 2020

View reviewed changes

ronag closed this Aug 1, 2020

codebytere mentioned this pull request Aug 10, 2020

v14.8.0 proposal #34704

Merged

ronag mentioned this pull request Aug 17, 2020

Performance requests nodejs/llhttp#64

Closed

This was referenced Aug 25, 2020

Node.js 14.x integration tests are failing on response size asserts macbre/phantomas#769

Merged

Use nginx to make integration tests more deterministic macbre/phantomas#773

Merged

atian25 mentioned this pull request Sep 7, 2020

fix: only set keep-alive header before Node.js 14.8.0 eggjs/egg#4457

Merged

4 tasks

killagu reviewed Sep 7, 2020

View reviewed changes

This was referenced Sep 8, 2020

fix: only set keep-alive when not exists #35096

Closed

fix: only set keep-alive when not exists #35138

Closed

codebytere mentioned this pull request Sep 28, 2020

v12.19.0 proposal #35401

Merged

rickyes mentioned this pull request Nov 27, 2020

fix: skip specified Node.js version to set keep-alive eggjs/egg#4527

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

http: provide keep-alive timeout response header#34561

http: provide keep-alive timeout response header#34561
ronag wants to merge 3 commits intonodejs:masterfrom
nxtedition:keep-alive-hint

ronag commented Jul 30, 2020

Uh oh!

ronag commented Jul 30, 2020

Uh oh!

ronag Jul 30, 2020

Uh oh!

ronag Jul 30, 2020

Uh oh!

ronag commented Jul 30, 2020

Uh oh!

nodejs-github-bot commented Jul 30, 2020

Uh oh!

nodejs-github-bot commented Jul 30, 2020

Uh oh!

mcollina left a comment

Uh oh!

ronag commented Aug 1, 2020

Uh oh!

killagu Sep 7, 2020

Uh oh!

ronag Sep 7, 2020

Uh oh!

atian25 Sep 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

Uh oh!

Conversation

ronag commented Jul 30, 2020

Checklist

Uh oh!

ronag commented Jul 30, 2020

Uh oh!

ronag Jul 30, 2020

Choose a reason for hiding this comment

Uh oh!

ronag Jul 30, 2020

Choose a reason for hiding this comment

Uh oh!

ronag commented Jul 30, 2020

Uh oh!

nodejs-github-bot commented Jul 30, 2020

Uh oh!

nodejs-github-bot commented Jul 30, 2020

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

ronag commented Aug 1, 2020

Uh oh!

killagu Sep 7, 2020

Choose a reason for hiding this comment

Uh oh!

ronag Sep 7, 2020

Choose a reason for hiding this comment

Uh oh!

atian25 Sep 8, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants