Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Open
jhansen-tt wants to merge 1 commit intomscdex:master from
jhansen-tt:socketFix
Open

Connection.js: socket needs to be set up in the case of tls #472
jhansen-tt wants to merge 1 commit intomscdex:master from
jhansen-tt:socketFix

Conversation

@jhansen-tt
Copy link

@jhansen-tt jhansen-tt commented May 20, 2015

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

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

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

Copy link
Author

It explains a lot of weirdness :)

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.

Copy link
Author

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

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.

Copy link
Owner

mscdex commented Oct 24, 2015

@evarsanyi It's the socket option listed here.

Copy link

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

Copy link

armw4 commented Jan 17, 2016

This going to get merged?

halimov-oa and msimerson reacted with eyes emoji

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /