-
-
Notifications
You must be signed in to change notification settings - Fork 7.3k
Add missing options argument to createSecurePair.
#25109
Conversation
Allow wrapping TLSSocket inside another TLSSocket, emulate it using SecurePair in legacy APIs. fix #6204
|
Great! May I ask you to add a test to not let this slip away again? |
|
Tests sems valid, but does not work...another bug is blocking my test...: @indutny need your help. Да, удобнее по-русски, кстати. Exactly the same is reproduced with code but Also, seems not reproducible in nodejs 0.10. Also maybe that: #7691 ? |
`onselect` is set on the `sniObject_` not on the `Connection` instance. See: nodejs/node-v0.x-archive#25109
|
@socketpair there is a bug that should be fixed by nodejs/node#1720 |
|
cc @misterdjules please backport this if it sounds good to you! ;) |
|
Btw, why put this test into pummel folder? |
|
I don't know. I just see some tests about tls in that folder, and put my test in same folder. I can change PR if you say what to fix. |
|
I'd go with |
`onselect` is set on the `sniObject_` not on the `Connection` instance. See: #25109
|
I have cherry-picked commit from IOjs that fixed bug in C++ |
|
I have moved test from |
|
Now all tests passed! yay! Please merge! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if markdown syntax (link) is correct here
|
How to speedup merging? who is responsible? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, but we use 80 column limit everywhere. I may only suggest to convert this to hex and split it before it gets to 80 column. It will be much more compact this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
|
Sorry for delay! Just one minor nit, otherwise LGTM! |
Also helps in implementation of #6204, where some options passed to createSecurePair and ignored before this patch. These options are very helpful if someone wants to pass options.servername or options.SNICallback to securepair.
|
Hex encoding takess too much space. Just reformat decimal representation. |
`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]>
`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]>
|
@socketpair what do you think about reopening this PR for io.js master branch? I have just landed that fix commit that your PR depended upon. ;) |
|
I'm confused about everything, and forgot what is happening... :( There are nodejs, iojs and two patches in both projects. I want all fixes to be in both projects. What I should do so? |
|
@socketpair it can't go into v0.12, and so the next release of node is io.js :) I would suggest to open PR for io.js master ;) |
|
Don't forget to cc me there ;) |
|
@socketpair .. just to reiterate what @indutny said. This is not going to be able to land here. The right thing to do here would be to open a new PR against master on http://github.com/nodejs/node in order to get this landed. |
|
@socketpair .. no problem. I'll go ahead and close the PR here. Thanks! |
Also helps in implementation of #6204, where some ptions passed to
createSecurePair and ignored before this patch.
These options are very helpful if someone wants to pass
options.servername or options.SNICallback to securepair.