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: avoid retaining unneeded memory #11926

Closed
wants to merge 1 commit into from

Conversation

@lpinca
Copy link
Member

commented Mar 19, 2017

Prevents the events listeners of the sockets obtained with the HTTP upgrade mechanism from retaining unneeded objects like state.

Reduces memory usage by ~20%.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)
lib/_http_server.js Outdated
@@ -475,6 +473,7 @@ function onParserExecuteCommon(server, socket, parser, state, ret, d) {
socket.removeListener('data', state.onData);
socket.removeListener('end', state.onEnd);
socket.removeListener('close', state.onClose);
socket.removeAllListeners('drain');

This comment has been minimized.

Copy link
@lpinca

lpinca Mar 19, 2017

Author Member

I'm not sure if this is ok but there are 2 listeners for the drain event and both seem to be useless for upgrade requests.
The first checks if _httpMessage is set and this is never true for upgrade requests.
The second checks if the socket is _paused but unless I'm missing something this flag is never set for upgrade requests.

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Mar 20, 2017

Member

.removeAllListeners() also removes any user-installed 'drain' event listeners so no, probably not a good idea.

This comment has been minimized.

Copy link
@lpinca

lpinca Mar 20, 2017

Author Member

Yeah I get that but can a user add a listener for the drain event before this is called?

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Mar 20, 2017

Member

Yes, server.on('connection', c => c.on('drain', ondrain)).

@lpinca lpinca force-pushed the lpinca:free/unneeded-memory branch Mar 20, 2017

@jasnell

This comment has been minimized.

Copy link
Member

commented Mar 20, 2017

@cjihrig
Copy link
Contributor

left a comment

LGTM pending CI

@lpinca

This comment has been minimized.

Copy link
Member Author

commented Mar 20, 2017

Refs: #11868

lib/_http_server.js Outdated
@@ -475,6 +473,7 @@ function onParserExecuteCommon(server, socket, parser, state, ret, d) {
socket.removeListener('data', state.onData);
socket.removeListener('end', state.onEnd);
socket.removeListener('close', state.onClose);
socket.removeAllListeners('drain');

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Mar 20, 2017

Member

.removeAllListeners() also removes any user-installed 'drain' event listeners so no, probably not a good idea.

@lpinca lpinca force-pushed the lpinca:free/unneeded-memory branch Mar 20, 2017

@lpinca

This comment has been minimized.

Copy link
Member Author

commented Mar 20, 2017

@bnoordhuis updated, PTAL.
This listener is no longer removed as I can't find a clean way to selectively remove it without exporting the function.

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented Mar 20, 2017

@lpinca You could move it to lib/internal/http.js.

@lpinca

This comment has been minimized.

Copy link
Member Author

commented Mar 20, 2017

Done, thanks.

lib/internal/http.js Outdated
module.exports = {
outHeadersKey: Symbol('outHeadersKey')
outHeadersKey: Symbol('outHeadersKey'),
ondrain

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Mar 20, 2017

Member

I'd add a comma, saves one line in the diff next time someone adds a property.

@lpinca lpinca force-pushed the lpinca:free/unneeded-memory branch Mar 20, 2017

@jasnell

This comment has been minimized.

@jasnell

This comment has been minimized.

Copy link
Member

commented Mar 22, 2017

@lpinca ... this is not landing cleanly for some reason (even tho it shows zero conflicts...) can you please squash the commits, rebase, and I'll try again.

http: avoid retaining unneeded memory
Prevent the events listeners of the sockets obtained with the HTTP
upgrade mechanism from retaining unneeded memory.

Refs: #11868

@lpinca lpinca force-pushed the lpinca:free/unneeded-memory branch to 0100984 Mar 22, 2017

@lpinca

This comment has been minimized.

Copy link
Member Author

commented Mar 22, 2017

@jasnell done.

jasnell added a commit that referenced this pull request Mar 22, 2017
http: avoid retaining unneeded memory
Prevent the events listeners of the sockets obtained with the HTTP
upgrade mechanism from retaining unneeded memory.

Ref: #11868
PR-URL: #11926
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@jasnell

This comment has been minimized.

Copy link
Member

commented Mar 22, 2017

Landed in e0a9ad1

@jasnell jasnell closed this Mar 22, 2017

@lpinca lpinca deleted the lpinca:free/unneeded-memory branch Mar 22, 2017

@MylesBorins

This comment has been minimized.

Copy link
Member

commented Mar 28, 2017

This will need to be manually backported to v7.x

lpinca added a commit to lpinca/node that referenced this pull request Mar 29, 2017
http: avoid retaining unneeded memory
Prevent the events listeners of the sockets obtained with the HTTP
upgrade mechanism from retaining unneeded memory.

Ref: nodejs#11868
PR-URL: nodejs#11926
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@jasnell jasnell referenced this pull request Apr 4, 2017
@MylesBorins

This comment has been minimized.

Copy link
Member

commented Apr 10, 2017

Looks like this change is breaking spdy. Example breakage from spdy tests.

  6) SPDY Server spdy/3 plain mode should not crash on .writeHead() after socket close:
     Uncaught TypeError: Cannot read property 'emit' of null
      at Socket.socketOnError (_http_server.js:450:19)
      at Stream.onStreamError (lib/spdy/handle.js:110:18)
      at node_modules/spdy-transport/lib/spdy-transport/connection.js:749:12
      at Array.forEach (native)
      at Connection.destroyStreams (node_modules/spdy-transport/lib/spdy-transport/connection.js:745:33)
      at Socket.onclose (node_modules/spdy-transport/lib/spdy-transport/connection.js:185:10)
      at TCP._handle.close [as _onclose] (net.js:534:12)
@MylesBorins MylesBorins referenced this pull request Apr 10, 2017
@ronkorving

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2017

@indutny Any chance this could be solved on the spdy side of things?

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented Apr 11, 2017

Removing the backport-to-v7.x label and adding dont-land-on labels until this is resolved.

@lpinca

This comment has been minimized.

Copy link
Member Author

commented Apr 11, 2017

Not sure if the following is the proper way to fix the issue but it seems to work:

diff --git a/lib/spdy/server.js b/lib/spdy/server.js
index 777f682..d5321c2 100644
--- a/lib/spdy/server.js
+++ b/lib/spdy/server.js
@@ -189,6 +189,8 @@ proto._onStream = function _onStream (stream) {
     socket = new net.Socket(socketOptions)
   }
 
+  socket.server = this;
+
   handle.assignSocket(socket)
 
   // For v0.8

Will send a PR.

@lpinca

This comment has been minimized.

Copy link
Member Author

commented Apr 11, 2017

That patch seems to not work on Node.js 0.10 and 0.12.

The problem is that node-spdy calls the connectionListener on a new socket for which the server property is null.

@lpinca lpinca referenced this pull request Jun 9, 2017
3 of 3 tasks complete
lpinca added a commit to lpinca/node that referenced this pull request Jun 9, 2017
http: handle cases where socket.server is null
Fixes a regression that caused an error to be thrown when trying to
emit the 'timeout' event on the server referenced by `socket.server`.

Fixes: nodejs#13435
Refs: nodejs#11926
addaleax added a commit that referenced this pull request Jun 12, 2017
http: handle cases where socket.server is null
Fixes a regression that caused an error to be thrown when trying to
emit the 'timeout' event on the server referenced by `socket.server`.

Fixes: #13435
Refs: #11926
PR-URL: #13578
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax added a commit that referenced this pull request Jun 12, 2017
http: handle cases where socket.server is null
Fixes a regression that caused an error to be thrown when trying to
emit the 'timeout' event on the server referenced by `socket.server`.

Fixes: #13435
Refs: #11926
PR-URL: #13578
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.