-
Notifications
You must be signed in to change notification settings - Fork 30k
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
net: unref timer in parent sockets #891
Conversation
`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
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) |
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 wonder if it might be worth adding this as a helper function.
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.
Done. ;)
@@ -213,6 +213,8 @@ function TLSSocket(socket, options) { | |||
readable: false, | |||
writable: false | |||
}); | |||
if (socket) | |||
this._parent = socket; |
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 not combine the 2 following lines into one if
statement like:
if (socket)
this._parent = socket,
this._connecting = socket._connecting;
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.
Fixed.
LGTM if the tests pass |
`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>
Landed in 9b6b055, thank you! |
`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>
TLSSocket
wraps the originalnet.Socket
, but writes/reads to/fromTLSSocket
do not touch the timers of originalnet.Socket
.Introduce
socket._parent
property, and iterate through all parentsto unref timers and prevent timeout event on original
net.Socket
.Fix: nodejs/node-v0.x-archive#9242