FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

tls_wrap: migrate synchronous errors by joyeecheung · Pull Request #18125 · nodejs/node · GitHub

/ node Public

tls_wrap: migrate synchronous errors - #18125

Closed
joyeecheung wants to merge 3 commits into
nodejs:masterfrom
joyeecheung:tls-errors
ConversationCommits (3)ChecksFiles changed
Closed

tls_wrap: migrate synchronous errors#18125
joyeecheung wants to merge 3 commits into
nodejs:masterfrom
joyeecheung:tls-errors

Conversation

joyeecheung commented Jan 12, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

tls_wrap: migrate C++ errors to internal/errors.js

  • Throw ERR_TLS_SNI_FROM_SERVER when setting server names on a
    server-side socket instead of returning silently
  • Assert on wrap_->started and wrap_->ssl instead of throwing
    errors since these errors indicate that the user either uses
    private APIs, or monkey-patches internals, or hits a bug.

tls_wrap: migrate argument type-checking errors

  • Throw ERR_INVALID_ARG_TYPE from public APIs
  • Assert argument types in bindings instead of throwing errors
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
Affected core subsystem(s)

tls, errors

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. errors Issues and PRs related to JavaScript errors originated in Node.js core. tls Issues and PRs related to the tls subsystem. labels Jan 12, 2018
joyeecheung added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jan 12, 2018

joyeecheung commented Jan 12, 2018

Copy link
Copy Markdown
Member Author

cc @jasnell @nodejs/tsc

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

addaleax approved these changes Jan 12, 2018
View reviewed changes
Comment thread lib/_tls_wrap.js Outdated

addaleax Jan 12, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It just needs to be a StreamBase, even if LibuvStreamWrap is the common case here.

joyeecheung Jan 12, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@addaleax I thought about checking the type of handle here but there doesn't seem to be a way of checking those in JS land since there isn't a proper inheritance of StreamBase.

addaleax Jan 12, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@joyeecheung Yeah, externalStream doesn’t really tell you what kind of thing it is. :/

Ideally, what we could do is making the StreamBase inheritance hierarchy reflected in the actual JS handle classes; that requires more work than this PR, though…

joyeecheung Jan 12, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@addaleax Yeah that should probably better be done in a dedicated PR IMO

maclover7 reviewed Jan 12, 2018
View reviewed changes
Comment thread lib/_tls_wrap.js Outdated

maclover7 Jan 12, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Should these be throw errors.TypeErrors instead of asserts?

joyeecheung Jan 13, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@maclover7 I tried that before and the test looked really dubious. These are not user-facing errors, whoever hits then either is using the underscored API or patching internals incorrectly, or is hitting a bug. Either way assertions seem to be more appropriate.

indutny commented Jan 14, 2018 via email

Copy link
Copy Markdown
Member

tls: migrate argument type-checking errors
0cfb5ca 
* Throw ERR_INVALID_ARG_TYPE from public APIs
* Assert argument types in bindings instead of throwing errors

PR-URL: nodejs#18125
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
tls: migrate C++ errors to internal/errors.js
7e8ecc9 
* Throw ERR_TLS_SNI_FROM_SERVER when setting server names on a
  server-side socket instead of returning silently
* Assert on wrap_->started and wrap_->ssl instead of throwing
  errors since these errors indicate that the user either uses
  private APIs, or monkey-patches internals, or hits a bug.

PR-URL: nodejs#18125
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>

joyeecheung commented Jan 15, 2018

Copy link
Copy Markdown
Member Author

Updated since #17882 has landed and introduced a check on a changed error. The assertion is probably equally unhelpful as before but since tls.createSecurePair() is deprecated I think it's fine not to add another error code for it.

New CI: https://ci.nodejs.org/job/node-test-pull-request/12537/

joyeecheung mentioned this pull request Jan 15, 2018
Closed

joyeecheung commented Jan 15, 2018

Copy link
Copy Markdown
Member Author

CI is green. Can you take a loot again? Thanks! @addaleax @indutny

indutny approved these changes Jan 15, 2018
View reviewed changes

indutny left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM, as long as CI is happy

joyeecheung added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jan 15, 2018

joyeecheung commented Jan 16, 2018

Copy link
Copy Markdown
Member Author

Landed in f75bc2c...1c29da8, thanks!

joyeecheung added a commit that referenced this pull request Jan 16, 2018
tls: migrate argument type-checking errors
9ffebea 
* Throw ERR_INVALID_ARG_TYPE from public APIs
* Assert argument types in bindings instead of throwing errors

PR-URL: #18125
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
joyeecheung added a commit that referenced this pull request Jan 16, 2018
tls: migrate C++ errors to internal/errors.js
1c29da8 
* Throw ERR_TLS_SNI_FROM_SERVER when setting server names on a
  server-side socket instead of returning silently
* Assert on wrap_->started and wrap_->ssl instead of throwing
  errors since these errors indicate that the user either uses
  private APIs, or monkey-patches internals, or hits a bug.

PR-URL: #18125
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
joyeecheung removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jan 16, 2018
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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

addaleax addaleax approved these changes
jasnell jasnell approved these changes
+2 more reviewers
maclover7 maclover7 left review comments
indutny indutny approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. errors Issues and PRs related to JavaScript errors originated in Node.js core. 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL