Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

net: unref timer in parent sockets #891

Closed
wants to merge 3 commits into from
Closed

Conversation

indutny
Copy link
Member

@indutny indutny commented Feb 19, 2015

TLSSocket wraps the original net.Socket, but writes/reads to/from
TLSSocket do not touch the timers of original net.Socket.

Introduce socket._parent property, and iterate through all parents
to unref timers and prevent timeout event on original net.Socket.

Fix: nodejs/node-v0.x-archive#9242

`TLSSocket` wraps the original `net.Socket`, but writes/reads to/from
`TLSSocket` do not touch the timers of original `net.Socket`.

Introduce `socket._parent` property, and iterate through all parents
to unref timers and prevent timeout event on original `net.Socket`.

Fix: nodejs/node-v0.x-archive#9242
@indutny
Copy link
Member Author

indutny commented Feb 19, 2015

cc @iojs/crypto

@@ -445,7 +446,8 @@ Socket.prototype._destroy = function(exception, cb) {

this.readable = this.writable = false;

timers.unenroll(this);
for (var s = this; s !== null; s = s._parent)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it might be worth adding this as a helper function.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. ;)

@@ -213,6 +213,8 @@ function TLSSocket(socket, options) {
readable: false,
writable: false
});
if (socket)
this._parent = socket;
Copy link
Contributor

Choose a reason for hiding this comment

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

why not combine the 2 following lines into one if statement like:

if (socket)
  this._parent = socket,
  this._connecting = socket._connecting;

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@cjihrig
Copy link
Contributor

cjihrig commented Feb 19, 2015

LGTM if the tests pass

indutny added a commit that referenced this pull request Feb 19, 2015
`TLSSocket` wraps the original `net.Socket`, but writes/reads to/from
`TLSSocket` do not touch the timers of original `net.Socket`.

Introduce `socket._parent` property, and iterate through all parents
to unref timers and prevent timeout event on original `net.Socket`.

Fix: nodejs/node-v0.x-archive#9242
PR-URL: #891
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@indutny
Copy link
Member Author

indutny commented Feb 19, 2015

Landed in 9b6b055, thank you!

@indutny indutny closed this Feb 19, 2015
@indutny indutny deleted the fix/gh-9242 branch February 19, 2015 17:45
indutny added a commit to nodejs/node-v0.x-archive that referenced this pull request Feb 19, 2015
`TLSSocket` wraps the original `net.Socket`, but writes/reads to/from
`TLSSocket` do not touch the timers of original `net.Socket`.

Introduce `socket._parent` property, and iterate through all parents
to unref timers and prevent timeout event on original `net.Socket`.

Fix: #9242
PR-URL: nodejs/node#891
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@rvagg rvagg mentioned this pull request Feb 20, 2015
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants