tls: hard deprecate tls.createSecurePair #8783

yorkie · 2016-09-26T06:03:10Z

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

tls

Description of change

We should use util.deprecated to wrap the deprecated createSecurePair in tls module.

mscdex · 2016-09-26T07:32:22Z

This should be semver-major right?

yorkie · 2016-09-26T09:45:16Z

It's bug fix, so semver-patch is proper :)

imyller · 2016-09-26T10:45:26Z

lib/tls.js

-exports.createSecurePair = require('_tls_legacy').createSecurePair;
+exports.createSecurePair = internalUtil.deprecate(function() {
+  require('_tls_legacy').createSecurePair.apply(null, arguments);
+}, 'tls.createSecurePair is deprecated. Use tls.TLSSocket instead');


Very minor nit:
Looks like majority of existing deprecation warnings in core lib have period . after the last word instead

imyller

LGTM with nit fixed

mscdex · 2016-09-26T14:20:09Z

@nodejs/ctc What is the semver-ness of adding runtime (deprecation) warnings? I believe they can be handled/caught in-process now, but I wasn't sure if that changed policy or not?

MylesBorins · 2016-09-26T14:27:07Z

introducing new warnings has been treated as a semver major afaik /cc @jasnell

The number of breakages that have happened due to changing output on stderr has been surprising to say the least.

yorkie · 2016-09-26T16:20:29Z

Hi I have one question @thealphanerd, introducing new warnings even though it has been marked as deprecated in API docs should be treated as a major?

What I was thinking about is that this function should be with this warning once it's documented as deprecated API, but it doesn't, so this patch is fixing this bug in this way, and this pull-request is not going to deprecate anything else new.

addaleax · 2016-09-26T16:23:50Z

@yorkie Introducing new deprecation warnings is a semver-major change, yes, and it doesn’t need to happen just because something is documented as deprecated in the docs.

(Also, maybe relevant: #7964?)

addaleax · 2016-09-26T16:49:00Z

What is the semver-ness of adding runtime (deprecation) warnings? I believe they can be handled/caught in-process now

They can be handled using process.on('warning') but that won’t stop Node from writing them to stderr, unless --no-warnings is passed on the command line.

ChALkeR · 2016-09-26T18:10:21Z

@yorkie

Doc-deprecation does not always mean runtime-deprecation.

The current process for most API deprecations is to introduce a documentation-only deprecation (aka «soft-deprecate») in a major version, then to introduce a runtime-deprecation in code (aka «hard-deprecate») in some next major version, then remove the deprecated API in some next major version.

It's not a mistake here that it was deprecated in the docs but not in the runtime — it's the usual process. And hard-deprecation is a semver-major, this is not a bug fix.

Though it's most likely possible for 8.0, so let's keep this.

ChALkeR · 2016-09-26T18:11:37Z

Note: the deprecation process for createSecurePair was started in #6063.

jasnell · 2016-09-27T14:37:41Z

I don't believe we have a policy yet on the semver-iness of adding a normal process warning. Adding warnings has broken things in practice, however. Adding a deprecation warning, however, is definitely a semver-major.

This LGTM as a semver-major

yorkie · 2016-09-27T15:15:50Z

Okay, so what's the process of landing this one? Should we land this after v8 has been released?

/cc @ChALkeR @jasnell

jasnell · 2016-09-27T16:43:39Z

This would be able to land in master after at least two CTC members have signed off. Since the v7.x branch has already been cut, we would simply not pull it back to that branch. There's no reason to wait until after the v7.0.0 release goes out.

addaleax · 2016-09-27T20:15:07Z

Sorry, mis-clicked.

Judging from a quick look at #6063, it seems to be advantageous to move towards removing createSecurePair at some point; It would be great if somebody could write down what those advantages would be (and maybe include that in the commit message)?

yorkie · 2016-09-28T01:04:01Z

@addaleax What I can see is that the tls-legacy.js could be removed from source if we absolutely remove this function. But as @ChALkeR pointed out, should we warn in user-land and later remove it in next major version?

jasnell · 2016-09-28T13:12:36Z

I'd likely give it a couple of major versions before we considered removing it entirely. There's little harm in letting it sit for a while with the deprecation warning.

addaleax · 2016-09-30T15:17:31Z

@yorkie Sounds good! Would you mind adding something like The long-term goal here is removing lib/_tls_legacy.js entirely. to the commit message? :)

yorkie · 2016-10-05T02:10:48Z

Done @addaleax and sorry about missing for few days :(

addaleax

@yorkie Thanks! Don’t worry about it, semver-majors are usually not very urgent anyway.

LGTM

yorkie · 2016-10-05T15:27:49Z

Okay, now let's do a confirmation about LGTMs, the CTC @jasnell @addaleax have left LGTM in previous comments/statuses. So could we land this pull-request as semver-major?

Thanks :)

addaleax · 2016-10-05T16:27:38Z

So could we land this pull-request as semver-major?

Yup, formally all requirements are fulfilled. This change would likely be released in v8.0.0 in April, so there’s no urge in landing this, but if you want to you can.

yorkie · 2016-10-06T06:56:46Z

Okay, let's land this in v8.0.0 :)

jasnell · 2016-10-06T22:24:11Z

This can land now, the v7.x branch has already been cut. I just won't pull this commit over.

jasnell · 2016-10-06T22:25:00Z

Actually, you know what... this could use a quick test... just something that verifies that the deprecation warning is emitted.

jasnell · 2016-11-18T15:26:25Z

@yorkie ... can you add a test per my request here: #8783 (comment) . After that we can look to get it landed. Thank you!

MylesBorins · 2016-12-29T18:08:51Z

@yorkie want to get a test together so we can land this?

ChALkeR · 2016-12-31T12:01:49Z

@thealphanerd, @yorkie, remindier: the commit message is wrong here.

This in fact turns a soft (doc-only) deprecation into a hard (runtime) deprecation, but the commit message looks like it corrects an error in the previous deprecation step.

yorkie · 2017-01-06T04:59:52Z

@jasnell @MylesBorins Sorry for delay on testing this PR and added in the latest ef12e41. @ChALkeR Also updated the commit message :)

yorkie · 2017-01-06T06:07:54Z

Fixed some runtime errors :)

yorkie · 2017-01-06T06:11:48Z

And starting a CI: https://ci.nodejs.org/job/node-test-pull-request/5715

sam-github · 2017-01-09T19:18:19Z

PR doesn't touch the docs... it appears it is currently impossible to tell from the docs whether deprecation is doc-only, or runtime, which is not good.

I don't know, @jasnell, @addaleax , someone?, is there way to make this clear in the docs?

Is this something that can be addressed in this PR, for this API, or does it need more wide-spread changes in docs?

addaleax · 2017-01-10T00:57:41Z

@sam-github This is definitely the kind of information #10116 would provide in the docs

jasnell · 2017-01-10T03:30:31Z

@sam-github.... funny you should ask... I have a PR that provides a place to document deprecations that's still sitting waiting for some reviews. It would be excellent if I could get some clear LGTM's on it so I can get it landed. #10116

sam-github · 2017-01-10T19:31:25Z

@jasnell sorry, thought I'd reviewed your #10116, now I have.

yorkie · 2017-01-17T02:20:45Z

Any progress about this PR? requesting more reviews, thank you :)

sam-github

@yorkie LGTM, but I would suggest you write the commit message as "tls.createSecurePair()" --- note the trailing () indicating its a function, not a property

sam-github · 2017-01-17T03:19:12Z

@bnoordhuis @indutny Would be good to get a LGTM from someone who remembers when createSecurePair() was still used. @nodejs/crypto

yorkie · 2017-02-27T03:55:26Z

Ping... able to merge?

jasnell · 2017-02-27T03:58:34Z

Please hold off landing this one. I ended up with a largely duplicate pr that we need to reconcile.

jasnell · 2017-03-22T22:15:26Z

Closing this. Another PR deprecating this has landed in master

nodejs-github-bot added the tls Issues and PRs related to the tls subsystem. label Sep 26, 2016

imyller reviewed Sep 26, 2016

View reviewed changes

imyller approved these changes Sep 26, 2016

View reviewed changes

addaleax added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 26, 2016

addaleax closed this Sep 27, 2016

addaleax reopened this Sep 27, 2016

yorkie force-pushed the tls/warn-deprecated branch from aaedf57 to 7324067 Compare October 5, 2016 02:10

addaleax approved these changes Oct 5, 2016

View reviewed changes

yorkie force-pushed the tls/warn-deprecated branch from 7324067 to ef12e41 Compare January 6, 2017 04:58

nodejs-github-bot added lts-watch-v6.x labels Jan 6, 2017

yorkie changed the title ~~tls: should use .deprecated to wrap the deprecated createSecurePair~~ tls: hard deprecate tls.createSecurePair Jan 6, 2017

tls: deprecate tls.createSecurePair in runtime

36e3133

yorkie force-pushed the tls/warn-deprecated branch from ef12e41 to 36e3133 Compare January 6, 2017 06:06

nodejs-github-bot added lts-watch-v6.x labels Jan 6, 2017

sam-github approved these changes Jan 17, 2017

View reviewed changes

addaleax mentioned this pull request Feb 16, 2017

tls: runtime deprecate _tls_legacy #11349

Closed

3 tasks

jasnell closed this Mar 22, 2017

yorkie deleted the tls/warn-deprecated branch March 23, 2017 01:45

MylesBorins removed lts-watch-v4.x labels Nov 14, 2017

sam-github mentioned this pull request Sep 16, 2019

Undeprecate tls.createSecurePair #29559

Closed

tls: hard deprecate tls.createSecurePair #8783

tls: hard deprecate tls.createSecurePair #8783

Conversation

yorkie commented Sep 26, 2016

Checklist

Affected core subsystem(s)

Description of change

mscdex commented Sep 26, 2016

yorkie commented Sep 26, 2016

imyller Sep 26, 2016

Choose a reason for hiding this comment

imyller left a comment

Choose a reason for hiding this comment

mscdex commented Sep 26, 2016

MylesBorins commented Sep 26, 2016 • edited Loading

yorkie commented Sep 26, 2016 • edited Loading

addaleax commented Sep 26, 2016 • edited Loading

addaleax commented Sep 26, 2016

ChALkeR commented Sep 26, 2016 • edited Loading

ChALkeR commented Sep 26, 2016

jasnell commented Sep 27, 2016

yorkie commented Sep 27, 2016

jasnell commented Sep 27, 2016

addaleax commented Sep 27, 2016

yorkie commented Sep 28, 2016 • edited Loading

jasnell commented Sep 28, 2016

addaleax commented Sep 30, 2016

yorkie commented Oct 5, 2016

addaleax left a comment

Choose a reason for hiding this comment

yorkie commented Oct 5, 2016

addaleax commented Oct 5, 2016

yorkie commented Oct 6, 2016

jasnell commented Oct 6, 2016

jasnell commented Oct 6, 2016

jasnell commented Nov 18, 2016

MylesBorins commented Dec 29, 2016

ChALkeR commented Dec 31, 2016

yorkie commented Jan 6, 2017

yorkie commented Jan 6, 2017

yorkie commented Jan 6, 2017

sam-github commented Jan 9, 2017

addaleax commented Jan 10, 2017

jasnell commented Jan 10, 2017

sam-github commented Jan 10, 2017

yorkie commented Jan 17, 2017

sam-github left a comment

Choose a reason for hiding this comment

sam-github commented Jan 17, 2017

yorkie commented Feb 27, 2017

jasnell commented Feb 27, 2017

jasnell commented Mar 22, 2017

MylesBorins commented Sep 26, 2016 •

edited

Loading

yorkie commented Sep 26, 2016 •

edited

Loading

addaleax commented Sep 26, 2016 •

edited

Loading

ChALkeR commented Sep 26, 2016 •

edited

Loading

yorkie commented Sep 28, 2016 •

edited

Loading