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

Connection.js: socket needs to be set up in the case of tls #472

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jhansen-tt
Copy link

Otherwise the event handlers are set up on the wrong socket object.

…ls for all of the event handlers to work properly.
@dougturnkey
Copy link

Hmmm.... This might explain some weirdness I've been seeing since moving to node 0.12 against gmail?

@jhansen-tt
Copy link
Author

It explains a lot of weirdness :)

@mscdex
Copy link
Owner

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
Copy link
Author

I've only tested this on 0.12, but it works great there.

@evarsanyi
Copy link

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
Copy link
Owner

mscdex commented Oct 24, 2015

@evarsanyi It's the socket option listed here.

@evarsanyi
Copy link

Thanks, and thanks for the fix -- no more server crashes on transient DNS errors.

@armw4
Copy link

armw4 commented Jan 17, 2016

This going to get merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants