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: fixed socket destruction, timeout and leaking #2534

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
@tshemsedinov
Contributor

tshemsedinov commented Aug 25, 2015

  • 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/

@indutny

View changes

Show outdated Hide outdated lib/_http_server.js
@indutny

View changes

Show outdated Hide outdated lib/_http_server.js
@indutny

View changes

Show outdated Hide outdated lib/_http_server.js
@indutny

View changes

Show outdated Hide outdated lib/_http_server.js

@mscdex mscdex added the http label Aug 25, 2015

@ChALkeR ChALkeR added the memory label Aug 25, 2015

@ChALkeR

This comment has been minimized.

Show comment
Hide comment
@ChALkeR

ChALkeR Aug 25, 2015

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?

Member

ChALkeR commented Aug 25, 2015

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?

@tshemsedinov

This comment has been minimized.

Show comment
Hide comment
@tshemsedinov

tshemsedinov Aug 25, 2015

Contributor

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).

Contributor

tshemsedinov commented Aug 25, 2015

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).

@Fishrock123

View changes

Show outdated Hide outdated test/parallel/test-http-server-keep-alive-timeout.js
@evanlucas

View changes

Show outdated Hide outdated test/parallel/test-http-server-keep-alive-timeout.js
@Fishrock123

This comment has been minimized.

Show comment
Hide comment

olecom added a commit to olecom/supro that referenced this pull request Aug 27, 2015

@ChALkeR

This comment has been minimized.

Show comment
Hide comment

@ChALkeR ChALkeR referenced this pull request Sep 6, 2015

Closed

A weird commit from 1970 #2713

@indutny

View changes

Show outdated Hide outdated lib/_http_server.js
@indutny

View changes

Show outdated Hide outdated lib/_http_server.js
@indutny

View changes

Show outdated Hide outdated lib/_http_server.js
@indutny

View changes

Show outdated Hide outdated test/parallel/test-http-server-keep-alive-timeout.js
@indutny

View changes

Show outdated Hide outdated test/parallel/test-http-server-keep-alive-timeout.js
@indutny

This comment has been minimized.

Show comment
Hide comment
@indutny

indutny Sep 9, 2015

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!

Member

indutny commented Sep 9, 2015

@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!

@tshemsedinov

This comment has been minimized.

Show comment
Hide comment
@tshemsedinov

tshemsedinov Sep 10, 2015

Contributor

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?

Contributor

tshemsedinov commented Sep 10, 2015

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?

@indutny

This comment has been minimized.

Show comment
Hide comment
@indutny

indutny Sep 10, 2015

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).

Member

indutny commented Sep 10, 2015

@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).

@indutny

This comment has been minimized.

Show comment
Hide comment
@indutny

indutny Sep 10, 2015

Member

Thanks!

Member

indutny commented Sep 10, 2015

Thanks!

@tshemsedinov

View changes

Show outdated Hide outdated lib/_http_server.js
@indutny

View changes

Show outdated Hide outdated lib/_http_server.js
@ChALkeR

This comment has been minimized.

Show comment
Hide comment
@indutny

This comment has been minimized.

Show comment
Hide comment
@indutny

indutny Sep 10, 2015

Member

@ChALkeR : a bit premature? Maybe cancel?

Member

indutny commented Sep 10, 2015

@ChALkeR : a bit premature? Maybe cancel?

@ChALkeR

This comment has been minimized.

Show comment
Hide comment
@ChALkeR

ChALkeR Sep 10, 2015

Member

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

Member

ChALkeR commented Sep 10, 2015

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

@indutny

This comment has been minimized.

Show comment
Hide comment
@indutny

indutny Sep 10, 2015

Member

Ok then, let them warm up a bit :)

Member

indutny commented Sep 10, 2015

Ok then, let them warm up a bit :)

@tshemsedinov

This comment has been minimized.

Show comment
Hide comment
@tshemsedinov

tshemsedinov Sep 11, 2015

Contributor

Looks like everything works now but we have two issues: timeout events and renewing socket activity when it receives new requests while in keepAliveTimeout mode. I'll work on this tomorrow, thanks @indutny and @ChALkeR

Contributor

tshemsedinov commented Sep 11, 2015

Looks like everything works now but we have two issues: timeout events and renewing socket activity when it receives new requests while in keepAliveTimeout mode. I'll work on this tomorrow, thanks @indutny and @ChALkeR

@indutny

This comment has been minimized.

Show comment
Hide comment
@indutny

indutny Sep 11, 2015

Member

Thank you, @tshemsedinov !

Member

indutny commented Sep 11, 2015

Thank you, @tshemsedinov !

@Scratch-net

This comment has been minimized.

Show comment
Hide comment
@Scratch-net

Scratch-net Oct 6, 2015

Any progress on this?

Scratch-net commented Oct 6, 2015

Any progress on this?

@tshemsedinov

This comment has been minimized.

Show comment
Hide comment
@tshemsedinov

tshemsedinov Oct 6, 2015

Contributor

@Scratch-net, everything works, tests passed, but I see that we have conflicts with master branch, I'll resolve it today.

Contributor

tshemsedinov commented Oct 6, 2015

@Scratch-net, everything works, tests passed, but I see that we have conflicts with master branch, I'll resolve it today.

@Scratch-net

This comment has been minimized.

Show comment
Hide comment
@Scratch-net

Scratch-net Oct 7, 2015

@tshemsedinov Nice, thanks. But how do we get this into master? Looks like they forgot about this PR
@ChALkeR what do you think?

Scratch-net commented Oct 7, 2015

@tshemsedinov Nice, thanks. But how do we get this into master? Looks like they forgot about this PR
@ChALkeR what do you think?

@ChALkeR

This comment has been minimized.

Show comment
Hide comment
@ChALkeR

ChALkeR Oct 7, 2015

Member

@Scratch-net I don't think that I'm qualified for fully reviewing this.
@indutny is the one to be asked here.

Member

ChALkeR commented Oct 7, 2015

@Scratch-net I don't think that I'm qualified for fully reviewing this.
@indutny is the one to be asked here.

@trevnorris

This comment has been minimized.

Show comment
Hide comment
@trevnorris

trevnorris Oct 27, 2015

Contributor

Have there been new commits/force pushes since the latest CI?

Contributor

trevnorris commented Oct 27, 2015

Have there been new commits/force pushes since the latest CI?

@tshemsedinov

This comment has been minimized.

Show comment
Hide comment
@tshemsedinov

tshemsedinov Oct 27, 2015

Contributor

@trevnorris, no changes, just rebased

Contributor

tshemsedinov commented Oct 27, 2015

@trevnorris, no changes, just rebased

@trevnorris

This comment has been minimized.

Show comment
Hide comment
@trevnorris

trevnorris Oct 27, 2015

Contributor

LGTM. @indutny you have any objections?

Contributor

trevnorris commented Oct 27, 2015

LGTM. @indutny you have any objections?

@aqrln

This comment has been minimized.

Show comment
Hide comment
@tshemsedinov

This comment has been minimized.

Show comment
Hide comment
@tshemsedinov

tshemsedinov May 17, 2017

Contributor

We finally did it thanks to @aqrln !
It just requires squashing on landing.
Working on this PR we found differences in HTTPS behavior for freebsd in comparison with all other platforms (including osx). But that's beyond the scope of this PR and requires additional investigation. So we'll do it in the near future and publish as separate issue or PR.
@Trott @jasnell
pr-2534

Contributor

tshemsedinov commented May 17, 2017

We finally did it thanks to @aqrln !
It just requires squashing on landing.
Working on this PR we found differences in HTTPS behavior for freebsd in comparison with all other platforms (including osx). But that's beyond the scope of this PR and requires additional investigation. So we'll do it in the near future and publish as separate issue or PR.
@Trott @jasnell
pr-2534

@mcollina

LGTM

@fhinkel fhinkel removed the stalled label May 23, 2017

@fhinkel

This comment has been minimized.

Show comment
Hide comment
@fhinkel

fhinkel May 23, 2017

Member

CI from 5 days ago was green. Can you squash so we can land?

Member

fhinkel commented May 23, 2017

CI from 5 days ago was green. Can you squash so we can land?

@tshemsedinov

This comment has been minimized.

Show comment
Hide comment
@tshemsedinov

tshemsedinov May 23, 2017

Contributor

@fhinkel into 2 commits? My and @aqrln? Or into single one? What is a best practice if we have two contributors in one PR?

Contributor

tshemsedinov commented May 23, 2017

@fhinkel into 2 commits? My and @aqrln? Or into single one? What is a best practice if we have two contributors in one PR?

@fhinkel

This comment has been minimized.

Show comment
Hide comment
@fhinkel

fhinkel May 23, 2017

Member

Usually we squash everything into one commit. But if you want two commits for two authors I guess that's fine (as long as the tests are green on both commits for git bisect. Single commits just make it easier to backport and revert.

Member

fhinkel commented May 23, 2017

Usually we squash everything into one commit. But if you want two commits for two authors I guess that's fine (as long as the tests are green on both commits for git bisect. Single commits just make it easier to backport and revert.

@aqrln

This comment has been minimized.

Show comment
Hide comment
@aqrln

aqrln May 23, 2017

Member

@fhinkel @tshemsedinov I don't think it's feasible to split the PR into two commits with both of them passing tests in this case, because my commits basically fix the code so that it passes the tests. I'd just squash it into a single commit and list both authors in the full commit message. (The last thing is optional, though. I don't really mind whether I am listed as one of the authors.)

Member

aqrln commented May 23, 2017

@fhinkel @tshemsedinov I don't think it's feasible to split the PR into two commits with both of them passing tests in this case, because my commits basically fix the code so that it passes the tests. I'd just squash it into a single commit and list both authors in the full commit message. (The last thing is optional, though. I don't really mind whether I am listed as one of the authors.)

@aqrln aqrln closed this May 24, 2017

@aqrln

This comment has been minimized.

Show comment
Hide comment
@aqrln

aqrln May 24, 2017

Member

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

Member

aqrln commented May 24, 2017

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

@aqrln aqrln reopened this May 24, 2017

@aqrln

This comment has been minimized.

Show comment
Hide comment
@aqrln

aqrln May 24, 2017

Member

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

Member

aqrln commented May 24, 2017

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

@aqrln

This comment has been minimized.

Show comment
Hide comment
@aqrln

aqrln May 24, 2017

Member

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.)

Member

aqrln commented May 24, 2017

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.)

@aqrln

This comment has been minimized.

Show comment
Hide comment
@aqrln

aqrln May 24, 2017

Member

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

Member

aqrln commented May 24, 2017

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

@refack

Documentation generally LGTM.
Just a few nits and lite rephrasing

Show outdated Hide outdated doc/api/http.md
Show outdated Hide outdated doc/api/http.md
Show outdated Hide outdated doc/api/http.md
Show outdated Hide outdated doc/api/http.md
Show outdated Hide outdated doc/api/http.md
Show outdated Hide outdated doc/api/http.md
@aqrln

This comment has been minimized.

Show comment
Hide comment
@aqrln

aqrln May 25, 2017

Member

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

Member

aqrln commented May 25, 2017

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

@refack

refack approved these changes May 25, 2017

Docs LGTM.
Rubberstamp the code.

Show outdated Hide outdated doc/api/http.md
@aqrln

This comment has been minimized.

Show comment
Hide comment
@aqrln

aqrln May 25, 2017

Member

@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!

Member

aqrln commented May 25, 2017

@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!

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

This comment has been minimized.

Show comment
Hide comment
@aqrln

aqrln May 25, 2017

Member

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

Member

aqrln commented May 25, 2017

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

This comment has been minimized.

Show comment
Hide comment
@refack

refack approved these changes May 26, 2017

LGTM.
CITGM clean.

aqrln added a commit that referenced this pull request May 26, 2017

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

PR-URL: #2534
Author: Timur Shemsedinov <timur.shemsedinov@gmail.com>
Author: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@aqrln

This comment has been minimized.

Show comment
Hide comment
@aqrln

aqrln May 26, 2017

Member

Landed in 0aa7ef5 🎉

Member

aqrln commented May 26, 2017

Landed in 0aa7ef5 🎉

@aqrln aqrln closed this May 26, 2017

jasnell added a commit that referenced this pull request May 28, 2017

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

PR-URL: #2534
Author: Timur Shemsedinov <timur.shemsedinov@gmail.com>
Author: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>

@jasnell jasnell referenced this pull request May 28, 2017

Closed

8.0.0 Release Proposal #12220

@ChALkeR ChALkeR referenced this pull request Jun 9, 2017

Closed

http: fix timeout reset after keep-alive timeout #13549

3 of 3 tasks complete

aqrln added a commit to aqrln/node that referenced this pull request Jun 12, 2017

http: fix timeout reset after keep-alive timeout
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-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

http: fix timeout reset after keep-alive timeout
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 <refack@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>

MylesBorins added a commit that referenced this pull request Jun 13, 2017

http: fix timeout reset after keep-alive timeout
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 <refack@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>

MylesBorins added a commit that referenced this pull request Jun 13, 2017

http: fix timeout reset after keep-alive timeout
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 <refack@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Jan 15, 2018

Member

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

Member

gibfahn commented Jan 15, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment