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.

Sign up for GitHub

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

Jump to bottom

src: merge SSLWrap with TLSWrap #22344

Closed
wants to merge 2 commits into from
Closed

src: merge SSLWrap with TLSWrap #22344

wants to merge 2 commits into from

Conversation

maclover7
Copy link
Contributor

@maclover7 maclover7 commented Aug 15, 2018 •
edited
Loading

SSLWrap was introduced in b9a0eb0,
but the only other C++ class to "inherit" from SSLWrap went away in
9301b8a. Since all of this API is
already part of TLSWrap, move everything to that class. This also
helps move a decent amount of functionality out of node_crypto to
a "better" place within C++ land. Also, just personally, this extra layer of indirection kinda confused me.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@maclover7
src: merge SSLWrap with TLSWrap
8cb9854 
`SSLWrap` was introduced in b9a0eb0,
but the only other C++ class to "inherit" from `SSLWrap` went away in
9301b8a. Since all of this API is
already part of `TLSWrap`, move everything to that class. This also
helps move a decent amount of functionality out of `node_crypto` to
a "better" place within C++ land.
@nodejs-github-bot
Copy link
Collaborator

@maclover7 sadly an error occured when I tried to trigger a build :(

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Aug 15, 2018
@addaleax
Copy link
Member

@maclover7 This is … a big diff. The original conceptual distinction did make sense – SSLWrap contains everything related to properties of OpenSSL SSL streams, whereas TLSWrap is concerned with translating to the underlying stream and back (that the naming does not reflect this is indeed regrettable).

I’ve thought about doing this merge, but I think it mostly makes TLSWrap even more confusing than it already is…

@maclover7
Copy link
Contributor Author

@addaleax Yeah... I figured I'd open up this PR to at least have a discussion. Part of the issue is that the way things are currently broken up among the two files is kinda bad, but when you put everything together, it's somewhat better since an abstraction is gone, but also a little worse, since everything is in one big file now.

@apapirovski
Copy link
Member

I think having it in two files and separated makes sense in my mind. They do two different things and it makes a very complex part of the codebase a bit easier to understand, IMO.

@jasnell
Copy link
Member

jasnell commented Aug 16, 2018

Oy, hmmm... like @addaleax and @apapirovski ... I think I prefer them separated out.

@maclover7
Copy link
Contributor Author

Consensus is -1 for now, will close this out :)

@maclover7 maclover7 closed this Aug 24, 2018
@maclover7 maclover7 deleted the jm-mv-ssl-wrap branch August 24, 2018 14:27
@srl295 srl295 mentioned this pull request Oct 17, 2018
Merged
3 tasks
srl295 added a commit to srl295/node that referenced this pull request Oct 24, 2018
@srl295
tools, icu: actually failover if there are multiple URLs
d8f2d27 
Building on nodejs#23269, if multiple ICU download URLs are present, try the
next one in case of error.

Part of the ICU 63.1 bump, but independent code-wise.
nodejs#23244

PR-URL: nodejs#23715
Fixes: nodejs#22344
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
srl295 added a commit to srl295/node that referenced this pull request Oct 24, 2018
@srl295
deps: icu 63.1 bump (CLDR 34)
6786ff4 
- Full release notes: http://site.icu-project.org/download/63

Fixes: nodejs#22344

PR-URL: nodejs#23715
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos pushed a commit that referenced this pull request Oct 26, 2018
@srl295 @targos
tools, icu: actually failover if there are multiple URLs
c20eb4f 
Building on #23269, if multiple ICU download URLs are present, try the
next one in case of error.

Part of the ICU 63.1 bump, but independent code-wise.
#23244

PR-URL: #23715
Fixes: #22344
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos pushed a commit that referenced this pull request Oct 26, 2018
@srl295 @targos
deps: icu 63.1 bump (CLDR 34)
e5b51cc 
- Full release notes: http://site.icu-project.org/download/63

Fixes: #22344

PR-URL: #23715
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
hashseed pushed a commit to v8/node that referenced this pull request Oct 31, 2018
@srl295 @hashseed
deps: icu 63.1 bump (CLDR 34)
43a6704 
- Full release notes: http://site.icu-project.org/download/63

Fixes: nodejs#22344

PR-URL: nodejs#23715
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@srl295
Copy link
Member

srl295 commented Mar 2, 2019

^ oops, I had referenced this issue in commits instead of #23244 — sorry for the noise.

BethGriggs pushed a commit that referenced this pull request Apr 17, 2019
@srl295 @BethGriggs
tools, icu: actually failover if there are multiple URLs
fc18e6f 
Building on #23269, if multiple ICU download URLs are present, try the
next one in case of error.

Part of the ICU 63.1 bump, but independent code-wise.
#23244

PR-URL: #23715
Fixes: #22344
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
BethGriggs pushed a commit that referenced this pull request Apr 17, 2019
@srl295 @BethGriggs
deps: icu 63.1 bump (CLDR 34)
7a343db 
- Full release notes: http://site.icu-project.org/download/63

Fixes: #22344

PR-URL: #23715
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
BethGriggs pushed a commit that referenced this pull request Apr 28, 2019
@srl295 @BethGriggs
tools, icu: actually failover if there are multiple URLs
2b9c492 
Building on #23269, if multiple ICU download URLs are present, try the
next one in case of error.

Part of the ICU 63.1 bump, but independent code-wise.
#23244

PR-URL: #23715
Fixes: #22344
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
BethGriggs pushed a commit that referenced this pull request Apr 28, 2019
@srl295 @BethGriggs
deps: icu 63.1 bump (CLDR 34)
cd1939f 
- Full release notes: http://site.icu-project.org/download/63

Fixes: #22344

PR-URL: #23715
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 16, 2019
@srl295 @MylesBorins
tools, icu: actually failover if there are multiple URLs
3ded3df 
Building on #23269, if multiple ICU download URLs are present, try the
next one in case of error.

Part of the ICU 63.1 bump, but independent code-wise.
#23244

PR-URL: #23715
Fixes: #22344
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 16, 2019
@srl295 @MylesBorins
deps: icu 63.1 bump (CLDR 34)
e9a9c88 
- Full release notes: http://site.icu-project.org/download/63

Fixes: #22344

PR-URL: #23715
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
BaochengSu pushed a commit to BaochengSu/node that referenced this pull request Oct 20, 2020
@srl295 @BaochengSu
tools, icu: actually failover if there are multiple URLs
c1a860c 
Building on nodejs#23269, if multiple ICU download URLs are present, try the
next one in case of error.

Part of the ICU 63.1 bump, but independent code-wise.
nodejs#23244

PR-URL: nodejs#23715
Fixes: nodejs#22344
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
(cherry picked from commit d8f2d27)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@maclover7 @nodejs-github-bot @addaleax @apapirovski @jasnell @srl295