tls: make deprecated tls.createSecurePair() use public API by addaleax · Pull Request #17882 · nodejs/node · GitHub

Make the deprecated tls.createSecurePair() method use other public
APIs only (TLSSocket in particular).

Since tls.createSecurePair() has been runtime-deprecated only
since Node 8, it probably isn’t quite time to remove it yet,
but this patch removes almost all of the code complexity that
is retained by it.

The API, as it is documented, is retained. However, it is very likely
that some users have come to rely on parts of undocumented API
of the SecurePair class, especially since some of the existing
tests checked for those. Therefore, this should definitely be
considered a breaking change.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

tls

CI: https://ci.nodejs.org/job/node-test-commit/15097/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1176/
(both good)

I’m very open to just not considering this a good idea.

Nice! And that DuplexPair class is clever.

it is very likely that some users have come to rely on parts of undocumented API of the SecurePair class

Do you mean ._external? I have no problem with breaking that, that's always been an ill-considered hack.

You can remove sent_shutdown_string, received_shutdown_string and onnewsessiondone_string from env.h, this is the only place they're used.

onselect_string can be removed too.

@bnoordhuis Thanks! I removed the unused strings here.

Nice! And that DuplexPair class is clever.

It’s from our test suite. :) I’ll think about maybe making it a public API in a follow-up PR, it seems like a useful primitive to me.

it is very likely that some users have come to rely on parts of undocumented API of the SecurePair class

Do you mean ._external? I have no problem with breaking that, that's always been an ill-considered hack.

At least one of the removed tests here used e.g. pair.ssl.setSNICallback. Also, there’s no real reason not to consider this semver-major, right? :)

I think this is fantastic. As I don't do a full code review: rubberstamp LGTM.

Hmmm, I'm a little bit concerned about user breakage here. Since the function is deprecated and (fingers crossed?) can be removed in Node 10, is it worth it to rewrite the function if all of the deprecated code can "just" be removed soon-ish? Also, the irony of making a semver-major change on a deprecated function isn't lost on me yet 😄

@maclover7 The thing is, this PR brings us into a position where we don’t have an incentive to remove this feature that early, because its maintenance cost is reduced by 95 % (or at least that’s what the diff size says).

And since stability is good for Node core & I think that means we should always take our time with removing/deprecating features, this PR allows us to do exactly that.

Make the deprecated `tls.createSecurePair()` method use other public APIs only (`TLSSocket` in particular). Since `tls.createSecurePair()` has been runtime-deprecated only since Node 8, it probably isn’t quite time to remove it yet, but this patch removes almost all of the code complexity that is retained by it. The API, as it is documented, is retained. However, it is very likely that some users have come to rely on parts of undocumented API of the `SecurePair` class, especially since some of the existing tests checked for those. Therefore, this should definitely be considered a breaking change.

The code reduction is too good to pass up.

Landed in 9301b8a

Make the deprecated `tls.createSecurePair()` method use other public APIs only (`TLSSocket` in particular). Since `tls.createSecurePair()` has been runtime-deprecated only since Node 8, it probably isn’t quite time to remove it yet, but this patch removes almost all of the code complexity that is retained by it. The API, as it is documented, is retained. However, it is very likely that some users have come to rely on parts of undocumented API of the `SecurePair` class, especially since some of the existing tests checked for those. Therefore, this should definitely be considered a breaking change. PR-URL: #17882 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Minwoo Jung <minwoo@nodesource.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

I think it is fine but this landed with only one @nodejs/tsc member giving a LG.

Oh, right, thanks for catching … I kinda still had it in my mind that @bnoordhuis was one of the TSC people. Want me to rebase it out of master?

No, as I said: I personally think it is fine and I know that it is sometimes difficult to get enough people having a look.

LGTM after the fact

addaleax added 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. labels Dec 27, 2017

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Dec 27, 2017

bnoordhuis approved these changes Dec 27, 2017

View reviewed changes

jasnell approved these changes Dec 27, 2017

View reviewed changes

JungMinu approved these changes Dec 28, 2017

View reviewed changes

BridgeAR approved these changes Dec 28, 2017

View reviewed changes

addaleax force-pushed the tls-remove-legacy branch from fbb6e4e to 6123768 Compare December 31, 2017 21:39

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

arthurschreiber mentioned this pull request Jan 6, 2018

tls.createSecurePair is being runtime-deprecated tediousjs/tedious#515

Closed

addaleax force-pushed the tls-remove-legacy branch from 6123768 to eeae308 Compare January 6, 2018 12:11

TimothyGu approved these changes Jan 13, 2018

View reviewed changes

addaleax removed lib / src Issues and PRs related to general changes in the lib or src directory. author ready PRs that have at least one approval, no outstanding review comments, and a CI started. labels Jan 14, 2018

addaleax closed this Jan 14, 2018

addaleax deleted the tls-remove-legacy branch January 14, 2018 13:54

joyeecheung mentioned this pull request Jan 15, 2018

tls_wrap: migrate synchronous errors #18125

Closed

4 tasks

This was referenced Apr 20, 2018

Add an easy way to start a TLS communication on top of a plain *server side* Socket #13368

Closed

Performance Regression: tls.createSecurePair() Not Buffering Connections in v10 #20263

Closed

tls: make deprecated tls.createSecurePair() use public API by addaleax · Pull Request #17882 · nodejs/node · GitHub

Uh oh!

Uh oh!

tls: make deprecated tls.createSecurePair() use public API - #17882

tls: make deprecated tls.createSecurePair() use public API#17882addaleax wants to merge 1 commit intonodejs:masternodejs/node:masterfrom addaleax:tls-remove-legacyaddaleax/node:tls-remove-legacyCopy head branch name to clipboard

Conversation

addaleax commented Dec 27, 2017

Checklist

Affected core subsystem(s)

Uh oh!

addaleax commented Dec 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Dec 27, 2017

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Dec 27, 2017

Choose a reason for hiding this comment

Uh oh!

addaleax commented Dec 28, 2017

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

maclover7 commented Dec 31, 2017

Uh oh!

addaleax commented Dec 31, 2017

Uh oh!

TimothyGu left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Jan 14, 2018

Uh oh!

BridgeAR commented Jan 14, 2018

Uh oh!

addaleax commented Jan 14, 2018

Uh oh!

BridgeAR commented Jan 14, 2018

Uh oh!

mcollina commented Jan 14, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

tls: make deprecated tls.createSecurePair() use public API#17882
addaleax wants to merge 1 commit into
nodejs:masterfrom
addaleax:tls-remove-legacy

addaleax commented Dec 27, 2017 •
edited
Loading