tls: make deprecated tls.createSecurePair() use public API #17882
Conversation
|
CI: https://ci.nodejs.org/job/node-test-commit/15097/ 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.
| int flags = SSL_get_shutdown(ssl_); | ||
|
|
||
| if (flags & SSL_SENT_SHUTDOWN) { | ||
| Local<String> sent_shutdown_key = ssl_env()->sent_shutdown_string(); |
There was a problem hiding this comment.
You can remove sent_shutdown_string, received_shutdown_string and onnewsessiondone_string from env.h, this is the only place they're used.
| } | ||
|
|
||
| Local<Object> obj = Object::New(env->isolate()); | ||
| obj->Set(env->onselect_string(), args[0]); |
There was a problem hiding this comment.
onselect_string can be removed too.
|
@bnoordhuis Thanks! I removed the unused strings here.
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.
At least one of the removed tests here used e.g. |
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.
|
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 |
|
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 |
Make the deprecated
tls.createSecurePair()method use other publicAPIs only (
TLSSocketin particular).Since
tls.createSecurePair()has been runtime-deprecated onlysince 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
SecurePairclass, especially since some of the existingtests checked for those. Therefore, this should definitely be
considered a breaking change.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
tls