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

crypto: fix legacy SNICallback #1720

Closed
wants to merge 1 commit into from

Conversation

indutny
Copy link
Member

@indutny indutny commented May 17, 2015

onselect is set on the sniObject_ not on the Connection instance.

See: nodejs/node-v0.x-archive#25109

@indutny
Copy link
Member Author

indutny commented May 17, 2015

cc @socketpair

@socketpair
Copy link
Contributor

mkey, what about tests :) ?

@mscdex mscdex added tls Issues and PRs related to the tls subsystem. crypto Issues and PRs related to the crypto subsystem. labels May 17, 2015
@indutny
Copy link
Member Author

indutny commented May 17, 2015

You actually already wrote one :) Haha.

@indutny
Copy link
Member Author

indutny commented May 29, 2015

@bnoordhuis cc

@bnoordhuis
Copy link
Member

@indutny Can you add a regression test?

Maybe use env->onselect_string() in Connection::SetSNICallback to make it more obvious that they're related.

Aside, is there a reason for the existence of sniObject_? I'm not sure why the function isn't a property of e.g. the Connection object.

`onselect` is set on the `sniObject_` not on the `Connection` instance.

See: nodejs/node-v0.x-archive#25109
PR-URL: nodejs#1720
Reviewed-By: Ben Noordhuis <[email protected]>
@indutny indutny force-pushed the fix/gh-node-25109 branch from 79d647d to 0f2a9f8 Compare July 22, 2015 20:30
@indutny
Copy link
Member Author

indutny commented Jul 22, 2015

Added a test, running CI.

@indutny
Copy link
Member Author

indutny commented Jul 22, 2015

indutny added a commit that referenced this pull request Jul 22, 2015
`onselect` is set on the `sniObject_` not on the `Connection` instance.

See: nodejs/node-v0.x-archive#25109
PR-URL: #1720
Reviewed-By: Ben Noordhuis <[email protected]>
@indutny
Copy link
Member Author

indutny commented Jul 22, 2015

Landed in eb35968, thank you everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants