Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tls: doc-only deprecate new TLSSocket constructor #36711

Closed
wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Dec 31, 2020

Fixes: #10555
Refs: #10846

The new tls.TLSSocket() constructor does not set up all of the
necessary lifecycle management or event handlers necessary for
proper use. The tls.connect() method really should be the way
that all tls.TLSSocket() instances are created. This commit
begins the eventual phasing out of the new tls.TLSSocket()
constructor with a doc-only deprecation.

Signed-off-by: James M Snell [email protected]

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

Fixes: nodejs#10555
Refs: nodejs#10846

The `new tls.TLSSocket()` constructor does not set up all of the
necessary lifecycle management or event handlers necessary for
proper use. The `tls.connect()` method really should be the way
that all `tls.TLSSocket()` instances are created. This commit
begins the eventual phasing out of the `new tls.TLSSocket()`
constructor with a doc-only deprecation.

Signed-off-by: James M Snell <[email protected]>
@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Dec 31, 2020
@mscdex
Copy link
Contributor

mscdex commented Jan 1, 2021

How are we supposed to create an unconnected socket then? Why can't the "lifecycle management or event handlers" be done once the socket's connect() has been called or similar?

@jasnell
Copy link
Member Author

jasnell commented Jan 1, 2021

Well, this is only a doc only deprecation for now so the current way continues to work. What I would suggest is needed is an additional api or option to connect that allows creating the unconnected socket.

@jasnell jasnell added deprecations Issues and PRs related to deprecations. semver-major PRs that contain breaking changes and should be released in the next major version. labels Jan 2, 2021
@jasnell jasnell added the review wanted PRs that need reviews. label Jan 15, 2021
@jasnell
Copy link
Member Author

jasnell commented Jan 22, 2021

Given the lack of attention it appears there's no support for this yet.

@mildsunrise
Copy link
Member

Sorry, I've been pretty busy lately. Maybe I'm missing things, but I'm initially -1 on this since tls.connect() is only for creating client sockets. tls.connect() is too specialized / high level to generalize it into server sockets as well IMO, and asking users to create an unbound tls.Server and emit('connection') is inadequate as it doesn't expose the socket while the handshake takes place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecations Issues and PRs related to deprecations. doc Issues and PRs related to the documentations. review wanted PRs that need reviews. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tls: secureConnect event will not be emitted if TLSSocket is created via constructor instead of tls.connect
4 participants