TLS cleanups by sam-github · Pull Request #25861 · nodejs/node

sam-github · 2019-02-01T00:06:51Z

Minor TLS cleanups, mostly small refactors for clarity, and updates to documentation, with a few lib/_tls_wrap debug statements thrown in.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
documentation is changed or added
commit message follows commit guidelines

nodejs-github-bot · 2019-02-01T00:06:52Z

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

Declaration is unused, it was added by mistake in 46c5c33.

Make it clear which of the multiple interfaces a TLSWrap method is implementing by grouping and commenting the related methods.

`tls` shadows the global `tls` require, and isn't indicative of the arument type.

- Don't use both break and return simultaneously. - Use case:/UNREACHABLE() to enforce that all cases are handled, instead of CHECK().

The const_cast used to be necessary for SSL_get_app_data() in OpenSSL 0.9.7, but node doesn't compile against OpenSSL versions that old. However, now it's needed for the recently introduced SSL_renegotiate_pending(), which is not const-correct as of 1.1.1a.

sam-github · 2019-02-01T21:00:27Z

@nodejs/crypto

sam-github · 2019-02-05T15:43:42Z

@nodejs/collaborators please take a look

indutny

LGTM

lib/_tls_wrap.js

sam-github · 2019-02-05T18:15:32Z

@joyeecheung git node land says "No CI runs detected" for this, but as you can see, there are. I could just land it, but maybe you are interested in taking a look? EDIT: This isn't the first time I've seen this lately, the other times I just wrote them off as flakiness, but maybe its a more persistent issue, so I'll leave this as a demo. I'm using git node 1.14.0.

Trott · 2019-02-05T21:27:40Z

@sam-github You probably need to add a CI: comment. I just tried and it said "No CI runs detected". Let's see what happens if we add this:

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

Trott · 2019-02-05T21:28:14Z

^^^^ Yup, adding that comment fixed it.

sam-github · 2019-02-05T21:50:23Z

@Trott Thanks! I had no idea the comment was required.

sam-github · 2019-02-05T23:18:31Z

Landed in b1cc1af...62b4796.

Declaration is unused, it was added by mistake in 46c5c33. PR-URL: #25861 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Make it clear which of the multiple interfaces a TLSWrap method is implementing by grouping and commenting the related methods. PR-URL: #25861 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

`tls` shadows the global `tls` require, and isn't indicative of the arument type. PR-URL: #25861 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

- Don't use both break and return simultaneously. - Use case:/UNREACHABLE() to enforce that all cases are handled, instead of CHECK(). PR-URL: #25861 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

The const_cast used to be necessary for SSL_get_app_data() in OpenSSL 0.9.7, but node doesn't compile against OpenSSL versions that old. However, now it's needed for the recently introduced SSL_renegotiate_pending(), which is not const-correct as of 1.1.1a. PR-URL: #25861 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

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

Declaration is unused, it was added by mistake in 46c5c33. PR-URL: #25861 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Make it clear which of the multiple interfaces a TLSWrap method is implementing by grouping and commenting the related methods. PR-URL: #25861 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

addaleax · 2019-02-07T17:40:50Z

I also find the comment helpful as it often says “I intend to merge this” before actually doing so, and because it’s often the only way to easily look up outdated CI runs for a given PR.

- Don't use both break and return simultaneously. - Use case:/UNREACHABLE() to enforce that all cases are handled, instead of CHECK(). Backport-PR-URL: nodejs#25968 PR-URL: nodejs#25861 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

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

- Don't use both break and return simultaneously. - Use case:/UNREACHABLE() to enforce that all cases are handled, instead of CHECK(). Backport-PR-URL: #25968 PR-URL: #25861 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

PR-URL: #25861 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Backport-PR-URL: #25968 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

Declaration is unused, it was added by mistake in 46c5c33. PR-URL: nodejs#25861 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Make it clear which of the multiple interfaces a TLSWrap method is implementing by grouping and commenting the related methods. PR-URL: nodejs#25861 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

`tls` shadows the global `tls` require, and isn't indicative of the arument type. PR-URL: nodejs#25861 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

The const_cast used to be necessary for SSL_get_app_data() in OpenSSL 0.9.7, but node doesn't compile against OpenSSL versions that old. However, now it's needed for the recently introduced SSL_renegotiate_pending(), which is not const-correct as of 1.1.1a. PR-URL: nodejs#25861 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

- Don't use both break and return simultaneously. - Use case:/UNREACHABLE() to enforce that all cases are handled, instead of CHECK(). Backport-PR-URL: nodejs#25968 PR-URL: nodejs#25861 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

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 Feb 1, 2019

sam-github added 6 commits February 1, 2019 08:47

src: remove unused TLWrap::EnableTrace()

9381fd3

Declaration is unused, it was added by mistake in 46c5c33.

src: organize TLSWrap declarations by parent

5ed7ad3

Make it clear which of the multiple interfaces a TLSWrap method is implementing by grouping and commenting the related methods.

tls: don't shadow the tls global with a local

25b6406

`tls` shadows the global `tls` require, and isn't indicative of the arument type.

src: refactor SSLError case statement

d295737

- Don't use both break and return simultaneously. - Use case:/UNREACHABLE() to enforce that all cases are handled, instead of CHECK().

tls: in-line comments and other cleanups

534379e

sam-github force-pushed the tls-cleanups branch from 1b4897a to 534379e Compare February 1, 2019 16:47

sam-github added the tls Issues and PRs related to the tls subsystem. label Feb 1, 2019

indutny approved these changes Feb 5, 2019

View reviewed changes

lib/_tls_wrap.js Show resolved Hide resolved

addaleax approved these changes Feb 5, 2019

View reviewed changes

sam-github closed this Feb 5, 2019

sam-github deleted the tls-cleanups branch February 5, 2019 23:18

sam-github added a commit that referenced this pull request Feb 5, 2019

tls: in-line comments and other cleanups

62b4796

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

sam-github mentioned this pull request Feb 7, 2019

git-node: detect CI status also from the GitHub API nodejs/node-core-utils#324

Closed

sam-github added a commit to sam-github/node that referenced this pull request Feb 7, 2019

tls: in-line comments and other cleanups

29f040a

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

targos added backported-to-v11.x and removed backport-requested-v11.x labels Feb 10, 2019

targos mentioned this pull request Feb 14, 2019

v11.10.0 proposal #26098

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

TLS cleanups#25861

TLS cleanups#25861
sam-github wants to merge 6 commits intonodejs:masterfrom
sam-github:tls-cleanups

sam-github commented Feb 1, 2019 •

edited

Loading

Uh oh!

nodejs-github-bot commented Feb 1, 2019

Uh oh!

sam-github commented Feb 1, 2019

Uh oh!

sam-github commented Feb 5, 2019

Uh oh!

indutny left a comment

Uh oh!

Uh oh!

sam-github commented Feb 5, 2019 •

edited

Loading

Uh oh!

Trott commented Feb 5, 2019

Uh oh!

Trott commented Feb 5, 2019

Uh oh!

sam-github commented Feb 5, 2019

Uh oh!

sam-github commented Feb 5, 2019 •

edited by Trott

Loading

Uh oh!

addaleax commented Feb 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Uh oh!

Conversation

sam-github commented Feb 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

nodejs-github-bot commented Feb 1, 2019

Uh oh!

sam-github commented Feb 1, 2019

Uh oh!

sam-github commented Feb 5, 2019

Uh oh!

indutny left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sam-github commented Feb 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Feb 5, 2019

Uh oh!

Trott commented Feb 5, 2019

Uh oh!

sam-github commented Feb 5, 2019

Uh oh!

sam-github commented Feb 5, 2019 • edited by Trott Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Feb 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

sam-github commented Feb 1, 2019 •

edited

Loading

sam-github commented Feb 5, 2019 •

edited

Loading

sam-github commented Feb 5, 2019 •

edited by Trott

Loading