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: provide keep-alive timeout response header #34561

Closed
wants to merge 3 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Jul 30, 2020

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 ronag requested a review from mcollina July 30, 2020 14:59
@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Jul 30, 2020
@ronag
Copy link
Member Author

ronag commented Jul 30, 2020

@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 ronag force-pushed the keep-alive-hint branch from d2c88ca to 9719ec1 Compare July 30, 2020 15:00
@ronag ronag force-pushed the keep-alive-hint branch from 686f8f2 to 003e581 Compare July 30, 2020 15:06
@@ -424,6 +427,10 @@ function _storeHeader(firstLine, headers) {
(state.contLen || this.useChunkedEncodingByDefault || this.agent);
if (shouldSendKeepAlive) {
header += 'Connection: keep-alive\r\n';
if (this._keepAliveTimeout) {
const timeoutSeconds = MathFloor(this._keepAliveTimeout) / 1000;
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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
Copy link
Member Author

ronag commented Jul 30, 2020

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

@nodejs-github-bot
Copy link
Collaborator

@ronag 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
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

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

@ronag
Copy link
Member Author

ronag commented Aug 1, 2020

Landed in 849d9e7

@ronag ronag closed this Aug 1, 2020
ronag added a commit that referenced this pull request Aug 1, 2020
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 <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Pranshu Srivastava <rexagod@gmail.com>
codebytere pushed a commit that referenced this pull request Aug 5, 2020
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 <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Pranshu Srivastava <rexagod@gmail.com>
@codebytere codebytere mentioned this pull request Aug 10, 2020
@@ -424,6 +427,10 @@ function _storeHeader(firstLine, headers) {
(state.contLen || this.useChunkedEncodingByDefault || this.agent);
if (shouldSendKeepAlive) {
header += 'Connection: keep-alive\r\n';
if (this._keepAliveTimeout) {
const timeoutSeconds = MathFloor(this._keepAliveTimeout) / 1000;
header += `Keep-Alive: timeout=${timeoutSeconds}\r\n`;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

PR welcome

Copy link
Contributor

Choose a reason for hiding this comment

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

rickyes pushed a commit that referenced this pull request Sep 12, 2020
PR-URL: #35138
Fixes: #34561
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
ruyadorno pushed a commit that referenced this pull request Sep 17, 2020
PR-URL: #35138
Fixes: #34561
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
addaleax pushed a commit that referenced this pull request Sep 22, 2020
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 <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Pranshu Srivastava <rexagod@gmail.com>
addaleax pushed a commit that referenced this pull request Sep 22, 2020
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 <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Pranshu Srivastava <rexagod@gmail.com>
@codebytere codebytere mentioned this pull request Sep 28, 2020
MylesBorins pushed a commit that referenced this pull request Nov 3, 2020
Backport-PR-URL: #35623
PR-URL: #35138
Fixes: #34561
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
MylesBorins pushed a commit that referenced this pull request Nov 16, 2020
Backport-PR-URL: #35623
PR-URL: #35138
Fixes: #34561
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
PR-URL: nodejs#35138
Fixes: nodejs#34561
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

http - keepalive timeout hint