-
Notifications
You must be signed in to change notification settings - Fork 390
Connection.js: socket needs to be set up in the case of tls#472
Connection.js: socket needs to be set up in the case of tls #472jhansen-tt wants to merge 1 commit intomscdex:master from
Conversation
...ls for all of the event handlers to work properly.
dougturnkey
commented
May 20, 2015
Hmmm.... This might explain some weirdness I've been seeing since moving to node 0.12 against gmail?
jhansen-tt
commented
May 20, 2015
It explains a lot of weirdness :)
mscdex
commented
Jun 10, 2015
We should probably make sure it still works for node v0.10 though and I remember some time back (not sure if it was 0.8 or 0.10) there were some events that were not propogated to the cleartext stream object and were only visible on the original socket. That is why it's currently coded the way it is.
If this change works (including error handling) on the latest node v0.10 release, then I have no problem merging it.
jhansen-tt
commented
Jun 10, 2015
I've only tested this on 0.12, but it works great there.
evarsanyi
commented
Oct 24, 2015
This fixed my issue not being able to catch errors on TLS connect (like connection refused or host not found on an itinerant connection). I originally hacked it locally by adding socket.on('error', _onError) but this is much cleaner and fixed other problems.
I can't find the docs in node's TLS module that say you're allowed to supply your own socket to connect at all.
I'm using 0.12.
mscdex
commented
Oct 24, 2015
@evarsanyi It's the socket option listed here.
evarsanyi
commented
Oct 25, 2015
Thanks, and thanks for the fix -- no more server crashes on transient DNS errors.
armw4
commented
Jan 17, 2016
This going to get merged?
Otherwise the event handlers are set up on the wrong socket object.