tls: hard deprecate tls.createSecurePair

[yorkie]

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]

This should be semver-major right?

[yorkie]

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

[imyller]

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]

@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]

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]

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]

@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]

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]

@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]

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

[jasnell]

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]

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

/cc @ChALkeR @jasnell

[jasnell]

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]

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]

@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]

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]

@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]

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]

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]

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]

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

[jasnell]

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

[jasnell]

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

[jasnell]

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

[MylesBorins]

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

[ChALkeR]

@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]

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

[yorkie]

Fixed some runtime errors :)

[yorkie]

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

[sam-github]

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]

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

[jasnell]

@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]

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

[yorkie]

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]

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

[yorkie]

Ping... able to merge?

[jasnell]

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

[jasnell]

Closing this. Another PR deprecating this has landed in master