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.

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

tls: de-duplicate for TLSSocket methods #22142

Closed
wants to merge 1 commit into from

Conversation

maclover7
Copy link
Contributor

Similar approach is used for TLSWrap, where C++ handle methods are
mapped one-to-one in JS.

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

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot nodejs-github-bot added the tls Issues and PRs related to the tls subsystem. label Aug 5, 2018
Similar approach is used for `TLSWrap`, where C++ handle methods are
mapped one-to-one in JS.
@maclover7
Copy link
Contributor Author

} else {
// Proxy TLSSocket handle methods
function makeSocketMethodProxy(name) {
return function socketMethodProxy(...args) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this show up in stack traces now instead of the actual proxied function name? I think having the actual function name in the stack trace would be more helpful.

Copy link
Member

@joyeecheung joyeecheung Aug 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

V8 is able to infer the method names. This should show up as TLSSocket.socketMethodProxy <as getFinished> (previously TLSSocket.getFinished)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, like @joyeecheung is saying, if an error was thrown in getFinished it would now appear as at TLSSocket.socketMethodProxy [as getFinished]

@Trott
Copy link
Member

Trott commented Aug 5, 2018

@nodejs/security-wg

@joyeecheung
Copy link
Member

By the way this does alter the return values of certain methods from undefined to null (when this._handle is not there)

@Trott
Copy link
Member

Trott commented Aug 7, 2018

By the way this does alter the return values of certain methods from undefined to null (when this._handle is not there)

semver-major out of caution? Bug to be fixed in the refactor?

@maclover7
Copy link
Contributor Author

semver-major out of caution? Bug to be fixed in the refactor?

Will label semver-major just out of caution, in the unlikely chance someone's doing a === undefined or === null

@maclover7 maclover7 added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 8, 2018
@maclover7
Copy link
Contributor Author

@mscdex Is your comment a blocking -1 (can you use the "request changes" review option if so)?

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@maclover7
Copy link
Contributor Author

ping @mscdex

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 5, 2018
@addaleax
Copy link
Member

Landed in 3c2aa4b

@addaleax addaleax closed this Sep 17, 2018
addaleax pushed a commit that referenced this pull request Sep 17, 2018
Similar approach is used for `TLSWrap`, where C++ handle methods are
mapped one-to-one in JS.

PR-URL: #22142
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. semver-major PRs that contain breaking changes and should be released in the next major version. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants