Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

tls: do not hang without newSession handler #25739

Closed
wants to merge 2 commits into from

Conversation

indutny
Copy link
Member

@indutny indutny commented Jul 18, 2015

When listening for client hello parser events (like OCSP requests), do
not hang if newSession event handler is not present.

When listening for client hello parser events (like OCSP requests), do
not hang if `newSession` event handler is not present.

Fix: nodejs#8660
Fix: nodejs#25735
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: node-forward/node#47
@indutny
Copy link
Member Author

indutny commented Jul 18, 2015

cc @joyent/node-collaborators

});

// Should not be actually called
server.on('resumeSession', function (id, callback) {

Choose a reason for hiding this comment

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

Can we use common.mustCall(function() {}, 0)?

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.

@tlivings
Copy link

Can we get some action on this - it's a pressing concern for us.

@sam-github
Copy link

LGTM

@indutny Does this effect io.js, too?

@tlivings Wouldn't a workaround be to just establish a newSession listener? You don't have to save the session, just callback immediately.

@indutny
Copy link
Member Author

indutny commented Jul 20, 2015

@sam-github it is effectively a backport from io.js, except the new test case. @sam-github should I just land the PR by hand as we do in io.js?

@tlivings
Copy link

@sam-github simply adding a listener doesn't work since we use SSL_OP_NO_TICKET. See also issue: #25736

@indutny
Copy link
Member Author

indutny commented Jul 20, 2015

@sam-github
Copy link

@indutny you've committed way more to joyent/node than I have!

I don't think the procedure is changed much from when you were working all the time over here, but I don't recall from the onboarding, and can't find anything specific to how to land in https://nodejs.org/contribute/code_contributions/workflow.html

@misterdjules ?

@indutny
Copy link
Member Author

indutny commented Jul 20, 2015

Cancelled that CI job, new one is:http://jenkins.nodejs.org/job/node-accept-pull-request/160/

@indutny
Copy link
Member Author

indutny commented Jul 20, 2015

Thanks @sam-github figured it out with @misterdjules

indutny added a commit that referenced this pull request Jul 20, 2015
When listening for client hello parser events (like OCSP requests), do
not hang if `newSession` event handler is not present.

Fix: #8660
Fix: #25735

Reviewed-By: Fedor Indutny <fedor@indutny.com>
PR-URL: #25739
indutny added a commit that referenced this pull request Jul 20, 2015
See: #25736

Reviewed-By: Fedor Indutny <fedor@indutny.com>
PR-URL: #25739
indutny added a commit that referenced this pull request Jul 20, 2015
Reviewed-By: Fedor Indutny <fedor@indutny.com>
PR-URL: #25739
@indutny
Copy link
Member Author

indutny commented Jul 20, 2015

Looks like it landed in v0.12.

@indutny indutny closed this Jul 20, 2015
jBarz pushed a commit to ibmruntimes/node that referenced this pull request Nov 4, 2016
When listening for client hello parser events (like OCSP requests), do
not hang if `newSession` event handler is not present.

Fix: nodejs#8660
Fix: nodejs#25735

Reviewed-By: Fedor Indutny <fedor@indutny.com>
PR-URL: nodejs#25739
jBarz pushed a commit to ibmruntimes/node that referenced this pull request Nov 4, 2016
See: nodejs#25736

Reviewed-By: Fedor Indutny <fedor@indutny.com>
PR-URL: nodejs#25739
jBarz pushed a commit to ibmruntimes/node that referenced this pull request Nov 4, 2016
Reviewed-By: Fedor Indutny <fedor@indutny.com>
PR-URL: nodejs#25739
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants