GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||
GeezerVison is showing github.com — http: fixed socket destruction, timeout and leaking by tshemsedinov · Pull Request #2534 · nodejs/node Conversation
Contributor
Memory usage comparison before and after: Socket handlers usage comparison before and after: Tests changed and provided new.
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
Contributor
Author
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
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
issue: nodejs/node#2534 crutch from: http://habrahabr.ru/post/264851/ Signed-off-by: Oleg Verych <[email protected]>
Member
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In debugging I found that when res was last in
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, it should be a separate clause for this thing.
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We checking that on
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
Contributor
Author
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
Member
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
Member
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
Member
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
Member
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
|
There was an error while loading. Please reload this page.
|
Wow, I think this wasn't a successful force-push :/ |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
That was strange, but I've reopened the PR and the commit is here, great. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
I wanted to trigger a fresh CI run on this, but something got really borked with this PR and
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
/s/renew/reset/
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@refack thanks! I've made some amendments, can you please take another look? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
|
@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! |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Implement server.keepAliveTimeout in addition to server.timeout to prevent temporary socket/memory leaking in keep-alive mode.
|
Squashed and rebased onto master again. I'd like to land it today or tomorrow. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/832/ |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
CITGM clean.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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]>
|
Landed in 0aa7ef5 🎉 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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]>
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
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]>
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]>
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]>
|
Release team decided not to backport to v6.x. Comment if you would like it to be backported. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.