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

Remove dead TLS code, and fix some misnamed variables #25153

Closed
wants to merge 5 commits into from
Closed

Remove dead TLS code, and fix some misnamed variables #25153

wants to merge 5 commits into from

Conversation

sam-github
Copy link
Contributor

@sam-github sam-github commented Dec 20, 2018 •
edited
Loading

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

@sam-github build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2019/pipeline

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Dec 20, 2018
@sam-github
Copy link
Contributor Author

ci: https://ci.nodejs.org/job/node-test-pull-request/19730/

@sam-github
Copy link
Contributor Author

@nodejs/crypto

@sam-github
Copy link
Contributor Author

ci: https://ci.nodejs.org/job/node-test-pull-request/19842/

@sam-github
tls: remove unused ocsp extension parsing
dd12dc9 
The OCSP info from parsing the TLS ClientHello has not been used since
550c263, remove it.

See: nodejs#1464
@sam-github
tls: fix initRead socket argument name
f309cec 
"wrapped" argument is the caller's "socket", not its "wrap", and its
referred to as "socket" in the comments, so call it that.
@sam-github
tls: do not confuse TLSSocket and Socket
2e526ce 
Don't use "socket" to describe two different objects in the same
function.
@sam-github
tls: do not confuse session and session ID
c25afe9 
session ID was named session in C++ and key in JS, Name them after what
they are, as the 'newSession' event docs do.
@sam-github
src: use consistent names for JSStream
fe15429 
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
Copy link
Contributor Author

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.

@sam-github
Copy link
Contributor Author

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/

@sam-github
Copy link
Contributor Author

Landed in acb49dc...00944c7

freebsd passed

@sam-github sam-github closed this Dec 28, 2018
@sam-github sam-github deleted the tls-cleanups-1 branch December 28, 2018 20:58
sam-github added a commit that referenced this pull request Dec 28, 2018
@sam-github
tls: remove unused ocsp extension parsing
08387b2 
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
@sam-github
tls: fix initRead socket argument name
59aa941 
"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
@sam-github
tls: do not confuse TLSSocket and Socket
b50c22a 
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
@sam-github
tls: do not confuse session and session ID
03e23a3 
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
@sam-github
src: use consistent names for JSStream
00944c7 
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
@sam-github @targos
tls: remove unused ocsp extension parsing
acf7802 
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
@sam-github @targos
tls: fix initRead socket argument name
d5ba121 
"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
@sam-github @targos
tls: do not confuse session and session ID
f6b2ea8 
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
@sam-github @targos
src: use consistent names for JSStream
bc66356 
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
@sam-github
lib: deprecate _stream_wrap
a1fb336 
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
@sam-github
lib: deprecate _stream_wrap
c7e628f 
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>
@sam-github sam-github mentioned this pull request Apr 26, 2019
Closed
BethGriggs pushed a commit that referenced this pull request Apr 28, 2019
@sam-github @BethGriggs
tls: do not confuse TLSSocket and Socket
41e16c1 
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
@sam-github
tls: fix initRead socket argument name
4a82835 
"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
@sam-github
tls: do not confuse session and session ID
78b42fc 
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
@sam-github
src: use consistent names for JSStream
a6635b2 
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
@sam-github
tls: remove unused ocsp extension parsing
ae7c74c 
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>
@BethGriggs BethGriggs mentioned this pull request May 1, 2019
Merged
BethGriggs pushed a commit that referenced this pull request May 10, 2019
@sam-github @BethGriggs
tls: do not confuse TLSSocket and Socket
678962e 
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
@sam-github @MylesBorins
tls: do not confuse TLSSocket and Socket
858a42e 
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
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tniessen tniessen tniessen approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

@apapirovski apapirovski apapirovski approved these changes

Assignees
No one assigned
Labels
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.
7 participants
@sam-github @nodejs-github-bot @addaleax @BridgeAR @apapirovski @jasnell @tniessen