GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — http: fixed socket destruction, timeout and leaking by tshemsedinov · Pull Request #2534 · nodejs/node

Conversation

@tshemsedinov

@tshemsedinov tshemsedinov commented Aug 25, 2015

Copy link
Copy Markdown
Contributor
  • Prevent 2 minutes sockets and memory hanging/leaking after server finished writing last response and connection hang inactive waiting for nothing (it is actual only in keep-alive mode, single HTTP requests does not hanging for 2 minutes, browsers do not reuse them and somehow do not close them too)
  • Fixed res._last, because it was not calculated correctly in this place before this PR and as a result socket.destroySoon() was never executed
  • Implemented server.keepAliveTimeout (default to 5 seconds) in addition to server.timeout
  • Prevent firing timeout event on server for finished and closed sockets
  • Removed socket.destroySoon() because it worked well on prefinish and have no sense on finish

Memory usage comparison before and after:
memory

Socket handlers usage comparison before and after:
sockets

Tests changed and provided new.
Related PR: #1411 by @indutny
Related Issue: nodejs/node-v0.x-archive#3460
Article about this problem (RU): http://habrahabr.ru/post/264851/

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
indutny
indutny reviewed Aug 25, 2015
View reviewed changes
Comment thread lib/_http_server.js Outdated

@indutny indutny Aug 25, 2015

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Unnecessary style fixes.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@mscdex mscdex added the http Issues and PRs related to the http subsystem. label Aug 25, 2015
@ChALkeR ChALkeR added the memory Issues and PRs related to Node.js memory management or memory footprint. label Aug 25, 2015
@ChALkeR

ChALkeR commented Aug 25, 2015

Copy link
Copy Markdown
Member

This PR changes the expectations of at least one test without changing anything else in that test. Does it make this PR a semver-major, or would it still be semver-minor?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@tshemsedinov

tshemsedinov commented Aug 25, 2015

Copy link
Copy Markdown
Contributor Author

This expectation https://github.com/tshemsedinov/io.js/blob/master/test/sequential/test-http-pipeline-flood.js#L51 based on wrong behavior, when timeout was emitted on sockets hanging open and on sockets already closed (it is actual just for keep-alive sockets).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Fishrock123
Fishrock123 reviewed Aug 25, 2015
View reviewed changes
Comment thread test/parallel/test-http-server-keep-alive-timeout.js Outdated

@Fishrock123 Fishrock123 Aug 25, 2015

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Please remove this license boilerplate. The necessary licensing can already be found in the LICENSE file.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@tshemsedinov tshemsedinov Aug 25, 2015

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Removed

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@Fishrock123

Fishrock123 commented Aug 25, 2015

Copy link
Copy Markdown
Contributor

CI: https://jenkins-iojs.nodesource.com/job/node-test-pull-request/163/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

olecom added a commit to olecom/supro that referenced this pull request Aug 27, 2015
@olecom
http: fixed socket destruction, timeout and leaking #2534 (in connect)
6069153
issue: nodejs/node#2534
crutch from: http://habrahabr.ru/post/264851/

Signed-off-by: Oleg Verych <[email protected]>
@brendanashworth brendanashworth mentioned this pull request Sep 2, 2015
Keep alive connection do not get closed with server.close() #2642
Closed
@ChALkeR

ChALkeR commented Sep 4, 2015

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@rvagg
rvagg force-pushed the master branch from 11c25c2 to ba02bd0 Compare September 6, 2015 11:55
@ChALkeR ChALkeR mentioned this pull request Sep 6, 2015
A weird commit from 1970 #2713
Closed
indutny
indutny reviewed Sep 9, 2015
View reviewed changes
Comment thread lib/_http_server.js Outdated

@indutny indutny Sep 9, 2015

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Why is this required?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@tshemsedinov tshemsedinov Sep 9, 2015

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Because in this place _last is not calculated, so when length is 0, _last is not true and res will not processed as last.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@indutny indutny Sep 9, 2015

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Maybe it is not calculated because the res might be not the last here?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@tshemsedinov tshemsedinov Sep 9, 2015

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

In debugging I found that when res was last in outgoing, and _last flag is not correct, but we need it calculated correctly for next lenes, to destroy socket if it is last.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@indutny indutny Sep 9, 2015

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Again, it should be a separate clause for this thing. _last indicates that there are not going to be any requests on the TCP socket. While there definitely could be in case of outgoing.length === 0.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@tshemsedinov tshemsedinov Sep 9, 2015

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

We checking that on res:finish event, why it is not a right place to detect "is outgoing queue is empty?" In this place res._last inicates that we have no nore message to send.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@indutny indutny Sep 9, 2015

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Because we should destroy the socket immediately when the connection wasn't keep-alive, or when TCP connection has already ended.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@indutny

indutny commented Sep 9, 2015

Copy link
Copy Markdown
Member

@tshemsedinov : decided to do a proper write up, instead of in-line comments.

I like the idea of PR, but I think the way it interacts with existing logic could be potentially buggy. res._last is used to destroy the underlying socket when either:

  • incoming TCP connection has ended
  • request didn't have Connection: keep-alive

Instead of using it for different purpose, I suggest to do it without property it all. If there are no pending requests - let's call setTimeout with that keep alive timeout value. When the new request comes - setTimeout with regular timeout value should be called, to restore it.

Thanks for submitting this!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@tshemsedinov

tshemsedinov commented Sep 10, 2015

Copy link
Copy Markdown
Contributor Author

Of course we can check outgoing queue and not changing _last immediately call setTimeout for socket destruction but I thought it was the purpose of this flag _last. I'll fix that, @indutny
What should I do with ditto marks? It is unwonted or indifferent?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@indutny

indutny commented Sep 10, 2015

Copy link
Copy Markdown
Member

@tshemsedinov there are lots of irrelevant style changes through the PR, ditto means "here too". Please change only the parts of code that are relevant to this behavior change. Also, there are some style issues (mostly 80 column limit).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@indutny

indutny commented Sep 10, 2015

Copy link
Copy Markdown
Member

Thanks!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

tshemsedinov
tshemsedinov reviewed Sep 10, 2015
View reviewed changes
Comment thread lib/_http_server.js Outdated

@tshemsedinov tshemsedinov Sep 10, 2015

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I found problem with non keep-alive sockets and here is fix for this case. @indutny, finally _last in it's untouched state is useful to detect non keep-alive sockets and destroy them as it was before this changes. Now all tests completed successfully.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@indutny indutny Sep 10, 2015

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Cool!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@ChALkeR

ChALkeR commented Sep 10, 2015

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@indutny

indutny commented Sep 10, 2015

Copy link
Copy Markdown
Member

@ChALkeR : a bit premature? Maybe cancel?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ChALkeR

ChALkeR commented Sep 10, 2015

Copy link
Copy Markdown
Member

@indutny He asked for it, why not? Most machines are free.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@aqrln

aqrln commented May 24, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Wow, I think this wasn't a successful force-push :/
Why did GitHub close this?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@aqrln aqrln reopened this May 24, 2017
@aqrln

aqrln commented May 24, 2017

Copy link
Copy Markdown
Contributor

That was strange, but I've reopened the PR and the commit is here, great.

ChALkeR reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@aqrln

aqrln commented May 24, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

I wanted to trigger a fresh CI run on this, but something got really borked with this PR and git fetch upstream +refs/pull/2534/head:pr-2534 fetches master instead of this commit, both locally on my machine and on CI, so I had to kill the job.

node-test-commit CI job for tshemsedinov/io.js instead: https://ci.nodejs.org/job/node-test-commit/10108/
(I rebased the commit onto the latest master before force-pushing to @tshemsedinov's repo, so that should be fine.)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@aqrln

aqrln commented May 24, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Yay, everything works fine after another force-push.

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

I've made some changes to the docs, namely: YAML header with the "added:" metadata, cross-links between the server.timeout and server.keepAliveTimeout sections, and a number of stylistic fixes. Can anyone review these, please? :)

/cc @jasnell @mcollina @fhinkel

tshemsedinov reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

refack
refack reviewed May 25, 2017
View reviewed changes

@refack refack left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Documentation generally LGTM.
Just a few nits and lite rephrasing

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread doc/api/http.md Outdated

@refack refack May 25, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Aren't these two bulleted list notes?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@aqrln aqrln May 25, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I'd rather not change it in this PR, it would be an unrelated stylistic change. But yeah, a nice thing to do in a follow-up PR.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@aqrln aqrln May 25, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Ah, never mind. I changed the server.timeout docs for consistency anyway.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

refack reacted with laugh emoji
All reactions
  • 😄 1 reaction
Comment thread doc/api/http.md Outdated

@refack refack May 25, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

[Suggestion] /s/Set to 0 to disable/A value of 0 will disable

P.S. what does it mean? Sockets will be destroyed ASAP, or timeout is delegated to OS?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@aqrln aqrln May 25, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

The latter option, roughly speaking. It will just disable any Node-specific timeout logic.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread doc/api/http.md Outdated

@refack refack May 25, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

IMHO you can simplify to just If [`server.keepAliveTimeout`][] is 0, the value of `server.timeout` will be used instead.
And now it's a Note: for server.keepAliveTimeout

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@aqrln aqrln May 25, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Hmm... or, maybe, even better to drop this sentence completely? It's not exactly accurate.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread doc/api/http.md Outdated

@refack refack May 25, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

* {Number} Timeout in milliseconds. Defaults to 5000.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

aqrln reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread doc/api/http.md Outdated

@refack refack May 25, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

The number of milliseconds of inactivity the server needs to wait for additional incoming data, after it has finished writing the last response, before a socket will be destroyed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

aqrln reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread doc/api/http.md Outdated

@refack refack May 25, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

/s/renew/reset/

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

aqrln reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@aqrln

aqrln commented May 25, 2017

Copy link
Copy Markdown
Contributor

@refack thanks! I've made some amendments, can you please take another look?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

refack
refack approved these changes May 25, 2017
View reviewed changes

@refack refack left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Docs LGTM.
Rubberstamp the code.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread doc/api/http.md Outdated

@refack refack May 25, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Tiny nit.
IMHO These two paragraphs could now be merged (the sentences have the same subject, so they could follow each other naturally)
very low importance on this

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

aqrln reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@aqrln

aqrln commented May 25, 2017

Copy link
Copy Markdown
Contributor

@jasnell as far as I see from the notifications, you're on GitHub now 😄 Can you please take a look at the latest changes while you are here? Thanks!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@tshemsedinov @aqrln
http: destroy sockets after keepAliveTimeout
b86b91f
Implement server.keepAliveTimeout in addition to server.timeout to
prevent temporary socket/memory leaking in keep-alive mode.
@aqrln

aqrln commented May 25, 2017

Copy link
Copy Markdown
Contributor

Squashed and rebased onto master again.
Fresh CI: https://ci.nodejs.org/job/node-test-pull-request/8313/

I'd like to land it today or tomorrow.

/cc @jasnell @mcollina @fhinkel @refack

refack and mcollina reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@refack

refack commented May 25, 2017

Copy link
Copy Markdown
Contributor

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/832/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

refack
refack approved these changes May 26, 2017
View reviewed changes

@refack refack left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM.
CITGM clean.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
aqrln pushed a commit that referenced this pull request May 26, 2017
@tshemsedinov @aqrln
http: destroy sockets after keepAliveTimeout
0aa7ef5
Implement server.keepAliveTimeout in addition to server.timeout to
prevent temporary socket/memory leaking in keep-alive mode.

PR-URL: #2534
Author: Timur Shemsedinov <[email protected]>
Author: Alexey Orlenko <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@aqrln

aqrln commented May 26, 2017

Copy link
Copy Markdown
Contributor

Landed in 0aa7ef5 🎉

denji reacted with thumbs up emoji seeden, denji, tshemsedinov, Mithgol, and eagleeye reacted with hooray emoji tshemsedinov and egoist reacted with heart emoji
All reactions
  • 👍 1 reaction
  • 🎉 5 reactions
  • ❤️ 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@aqrln aqrln closed this May 26, 2017
jasnell pushed a commit that referenced this pull request May 28, 2017
@tshemsedinov @jasnell
http: destroy sockets after keepAliveTimeout
a7f5c9c
Implement server.keepAliveTimeout in addition to server.timeout to
prevent temporary socket/memory leaking in keep-alive mode.

PR-URL: #2534
Author: Timur Shemsedinov <[email protected]>
Author: Alexey Orlenko <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@jasnell jasnell mentioned this pull request May 28, 2017
8.0.0 Release Proposal #12220
Closed
@brudil brudil mentioned this pull request Jun 1, 2017
net::ERR_INCOMPLETE_CHUNKED_ENCODING in node v8.0 + chrome 58.0.3029.110 webpack/webpack-hot-middleware#210
Closed
@pbininda pbininda mentioned this pull request Jun 2, 2017
Http connections aborted after 5s / keepAliveTimeout #13391
Closed
@ChALkeR ChALkeR mentioned this pull request Jun 9, 2017
http: fix timeout reset after keep-alive timeout #13549
Closed
3 tasks done
aqrln added a commit to aqrln/node that referenced this pull request Jun 12, 2017
@aqrln
http: fix timeout reset after keep-alive timeout
3d4d207
Fix the logic of resetting the socket timeout of keep-alive HTTP
connections and add two tests:

* `test-http-server-keep-alive-timeout-slow-server` is a regression test
  for nodejsGH-13391.  It ensures that the server-side keep-alive timeout will
  not fire during processing of a request.

* `test-http-server-keep-alive-timeout-slow-headers` ensures that the
  regular socket timeout is restored as soon as a client starts sending
  a new request, not as soon as the whole message is received, so that
  the keep-alive timeout will not fire while, e.g., the client is
  sending large cookies.

Refs: nodejs#2534
Fixes: nodejs#13391
aqrln added a commit that referenced this pull request Jun 13, 2017
@aqrln
http: fix timeout reset after keep-alive timeout
d71718d
Fix the logic of resetting the socket timeout of keep-alive HTTP
connections and add two tests:

* `test-http-server-keep-alive-timeout-slow-server` is a regression test
  for GH-13391.  It ensures that the server-side keep-alive timeout will
  not fire during processing of a request.

* `test-http-server-keep-alive-timeout-slow-client-headers` ensures that
  the regular socket timeout is restored as soon as a client starts
  sending a new request, not as soon as the whole message is received,
  so that the keep-alive timeout will not fire while, e.g., the client
  is sending large cookies.

Refs: #2534
Fixes: #13391
PR-URL: #13549
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Brian White <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jun 13, 2017
@aqrln @MylesBorins
http: fix timeout reset after keep-alive timeout
23318c7
Fix the logic of resetting the socket timeout of keep-alive HTTP
connections and add two tests:

* `test-http-server-keep-alive-timeout-slow-server` is a regression test
  for GH-13391.  It ensures that the server-side keep-alive timeout will
  not fire during processing of a request.

* `test-http-server-keep-alive-timeout-slow-client-headers` ensures that
  the regular socket timeout is restored as soon as a client starts
  sending a new request, not as soon as the whole message is received,
  so that the keep-alive timeout will not fire while, e.g., the client
  is sending large cookies.

Refs: #2534
Fixes: #13391
PR-URL: #13549
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Brian White <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jun 13, 2017
@aqrln @MylesBorins
http: fix timeout reset after keep-alive timeout
2cb6f2b
Fix the logic of resetting the socket timeout of keep-alive HTTP
connections and add two tests:

* `test-http-server-keep-alive-timeout-slow-server` is a regression test
  for GH-13391.  It ensures that the server-side keep-alive timeout will
  not fire during processing of a request.

* `test-http-server-keep-alive-timeout-slow-client-headers` ensures that
  the regular socket timeout is restored as soon as a client starts
  sending a new request, not as soon as the whole message is received,
  so that the keep-alive timeout will not fire while, e.g., the client
  is sending large cookies.

Refs: #2534
Fixes: #13391
PR-URL: #13549
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Brian White <[email protected]>
@sourcesoft sourcesoft mentioned this pull request Jul 5, 2017
Getting net::ERR_INCOMPLETE_CHUNKED_ENCODING react-boilerplate/react-boilerplate#1840
Closed
@cheton cheton mentioned this pull request Dec 2, 2017
Question: The "others" event callback Snapmaker/Luban#2
Closed
@gibfahn

gibfahn commented Jan 15, 2018

Copy link
Copy Markdown
Member

Release team decided not to backport to v6.x. Comment if you would like it to be backported.

tshemsedinov and dxhome reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ggoodman ggoodman mentioned this pull request Apr 24, 2018
https?.Server.keepAliveTimeout introduced boundary condition that results in client-side socket hang-ups #20256
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes
@jasnell jasnell jasnell approved these changes
+4 more reviewers
@aqrln aqrln aqrln left review comments
@refack refack refack approved these changes
@fhinkel fhinkel fhinkel approved these changes
@Prokky Prokky Prokky approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

http Issues and PRs related to the http subsystem. memory Issues and PRs related to Node.js memory management or memory footprint. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

20 participants

@tshemsedinov @ChALkeR @Fishrock123 @indutny @Scratch-net @trevnorris @vkurchatkin @rvagg @jasnell @Trott @Mithgol @mcollina @orangemocha @seeden @zbjornson @jbergstroem @usebaz @gibfahn @aqrln @fhinkel
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.