-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
src: refactor TLSWrap #35552
Conversation
Review requested:
|
93dfb6f
to
50dfd2a
Compare
Heh, some definite failures in CI ;-) ... moving this back to draft until I can take a look |
50dfd2a
to
7baee85
Compare
thank you! I was going to open a PR for this same thing |
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 <jasnell@gmail.com>
7baee85
to
963fe0f
Compare
@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.
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.
will this be backported? I initially assumed so, but it depends on a semver-major change, so 🤔 |
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