Remove dead TLS code, and fix some misnamed variables #25153
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
nodejs-github-bot added the lib / src 
sam-github force-pushed the tls-cleanups-1 branch from 4c9f10a to 3c4e3cc Compare 
| @nodejs/crypto |
addaleax approved these changes Dec 21, 2018
tniessen approved these changes Dec 22, 2018
jasnell approved these changes Dec 27, 2018
apapirovski approved these changes Dec 27, 2018
The OCSP info from parsing the TLS ClientHello has not been used since 550c263, remove it. See: nodejs#1464
"wrapped" argument is the caller's "socket", not its "wrap", and its referred to as "socket" in the comments, so call it that.
Don't use "socket" to describe two different objects in the same function.
session ID was named session in C++ and key in JS, Name them after what they are, as the 'newSession' event docs do.
Its confusing to call a js class with a handle a "Wrap", usually it's the C++ handle that is called a Wrap (tcp_wrap, tls_wrap, ...). Its derived from Socket, and makes a JS stream look like a Socket, so call it that. Also, remove use of lib/_stream_wrap.js so it can be deprecated some time.
sam-github force-pushed the tls-cleanups-1 branch from 3c4e3cc to fe15429 Compare | ci: https://ci.nodejs.org/job/node-test-pull-request/19847/ Last ci failed on one windows machine. Maybe ephemeral. Also, hard to read the logs to figure out why. Now its rebased on #25148 the log should be more readable if it does fail, so trying again. |
| Everything green except https://ci.nodejs.org/job/node-test-commit-freebsd/23089/, test.parallel/test-fs-read-stream-concurrent-reads, can't see how that is related to tls code removal. rebuild: https://ci.nodejs.org/job/node-test-commit-freebsd/23102/ |
| Landed in acb49dc...00944c7 freebsd passed |
sam-github added a commit that referenced this pull request Dec 28, 2018
The OCSP info from parsing the TLS ClientHello has not been used since 550c263, remove it. See: #1464 PR-URL: #25153 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
sam-github added a commit that referenced this pull request Dec 28, 2018
"wrapped" argument is the caller's "socket", not its "wrap", and its referred to as "socket" in the comments, so call it that. PR-URL: #25153 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
sam-github added a commit that referenced this pull request Dec 28, 2018
Don't use "socket" to describe two different objects in the same function. PR-URL: #25153 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
sam-github added a commit that referenced this pull request Dec 28, 2018
session ID was named session in C++ and key in JS, Name them after what they are, as the 'newSession' event docs do. PR-URL: #25153 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
sam-github added a commit that referenced this pull request Dec 28, 2018
Its confusing to call a js class with a handle a "Wrap", usually it's the C++ handle that is called a Wrap (tcp_wrap, tls_wrap, ...). Its derived from Socket, and makes a JS stream look like a Socket, so call it that. Also, remove use of lib/_stream_wrap.js so it can be deprecated some time. PR-URL: #25153 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
targos pushed a commit that referenced this pull request Jan 1, 2019
The OCSP info from parsing the TLS ClientHello has not been used since 550c263, remove it. See: #1464 PR-URL: #25153 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
targos pushed a commit that referenced this pull request Jan 1, 2019
"wrapped" argument is the caller's "socket", not its "wrap", and its referred to as "socket" in the comments, so call it that. PR-URL: #25153 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
targos pushed a commit that referenced this pull request Jan 1, 2019
session ID was named session in C++ and key in JS, Name them after what they are, as the 'newSession' event docs do. PR-URL: #25153 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
targos pushed a commit that referenced this pull request Jan 1, 2019
Its confusing to call a js class with a handle a "Wrap", usually it's the C++ handle that is called a Wrap (tcp_wrap, tls_wrap, ...). Its derived from Socket, and makes a JS stream look like a Socket, so call it that. Also, remove use of lib/_stream_wrap.js so it can be deprecated some time. PR-URL: #25153 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
sam-github added a commit to sam-github/node that referenced this pull request Mar 2, 2019
Its unused by node, and doesn't have a reasonable use outside of node. See: nodejs#25153 See: nodejs#16158
sam-github added a commit that referenced this pull request Mar 4, 2019
Its unused by node, and doesn't have a reasonable use outside of node. See: #25153 See: #16158 PR-URL: #26245 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>
BethGriggs pushed a commit that referenced this pull request Apr 28, 2019
Don't use "socket" to describe two different objects in the same function. PR-URL: #25153 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
sam-github added a commit to sam-github/node that referenced this pull request Apr 29, 2019
"wrapped" argument is the caller's "socket", not its "wrap", and its referred to as "socket" in the comments, so call it that. PR-URL: nodejs#25153 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
sam-github added a commit to sam-github/node that referenced this pull request Apr 29, 2019
session ID was named session in C++ and key in JS, Name them after what they are, as the 'newSession' event docs do. PR-URL: nodejs#25153 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
sam-github added a commit to sam-github/node that referenced this pull request Apr 29, 2019
Its confusing to call a js class with a handle a "Wrap", usually it's the C++ handle that is called a Wrap (tcp_wrap, tls_wrap, ...). Its derived from Socket, and makes a JS stream look like a Socket, so call it that. Also, remove use of lib/_stream_wrap.js so it can be deprecated some time. PR-URL: nodejs#25153 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
sam-github added a commit to sam-github/node that referenced this pull request Apr 29, 2019
The OCSP info from parsing the TLS ClientHello has not been used since 550c263, remove it. See: nodejs#1464 PR-URL: nodejs#25153 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Merged
BethGriggs pushed a commit that referenced this pull request May 10, 2019
Don't use "socket" to describe two different objects in the same function. PR-URL: #25153 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MylesBorins pushed a commit that referenced this pull request May 16, 2019
Don't use "socket" to describe two different objects in the same function. PR-URL: #25153 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
This was referenced May 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes