Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34.3k
src: refactor TLSWrap#35552
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
src: refactor TLSWrap #35552
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot commented Oct 8, 2020
Review requested:
|
nodejs-github-bot commented Oct 10, 2020
jasnell commented Oct 10, 2020
Heh, some definite failures in CI ;-) ... moving this back to draft until I can take a look |
50dfd2a to 7baee85Comparemildsunrise commented Oct 12, 2020
thank you! I was going to open a PR for this same thing |
jasnell commented Oct 12, 2020
Awesome. It's almost done. Have to trace down a couple of failing tests that aren't exactly obvious with regards to why they're failing but it's close |
SSLWrap was needlessly defined as a template class, splitting the TLS implementation over multiple locations. The original idea, I surmise, was to make it possible to reuse SSLWrap for some other purpose that never manifest. This squashes them down into a single TLSWrap class and moves tls_wrap.h/cc into src/crypto. Signed-off-by: James M Snell <[email protected]>
7baee85 to 963fe0fComparejasnell commented Oct 12, 2020
@mildsunrise (and others) this should be good to go now :-) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
jasnell commented Oct 13, 2020
Dealing with some seemingly unrelated CI failures on this. Running CI again to see what happens |
This comment has been minimized.
This comment has been minimized.
mildsunrise commented Oct 15, 2020
will this be backported? I initially assumed so, but it depends on a semver-major change, so 🤔 |
mildsunrise commented Oct 15, 2020 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
also, context for newer contributors who arrive here: |
This accomplishes a couple of things...
Previously, TLSWrap and SSLWrap were defined separately, with SSLWrap as an abstract template class presumably with the intent of allowing it to be reused in other ways that never actually happened. Here they are collapsed and SSLWrap is eliminated entirely
Updates and modernizes the code inside TLSWrap
While this is semver-patch, it depends on the currently semver-major #35093
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes