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: honor pauseOnConnect in tls.Server constructor #29632

Closed
wants to merge 4 commits into from
Closed

tls: honor pauseOnConnect in tls.Server constructor #29632

wants to merge 4 commits into from

Conversation

r1b
Copy link
Contributor

@r1b r1b commented Sep 20, 2019

#29620


Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

Calling the parent constructor in `tls.Server` is not sufficient to pass
along the `pauseOnConnect` option - it must also be passed when calling
the parent constructor in `TLSSocket`.

Tests were updated to cover the use of all possible net.Server options
in the tls.Server constructor.

Fixes: #29620
Refs: #27665
@nodejs-github-bot nodejs-github-bot added the tls Issues and PRs related to the tls subsystem. label Sep 20, 2019
@@ -411,6 +411,7 @@ function TLSSocket(socket, opts) {
net.Socket.call(this, {
handle: this._wrapHandle(wrap),
allowHalfOpen: socket ? socket.allowHalfOpen : tlsOptions.allowHalfOpen,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As written, tlsOptions.allowHalfOpen will never be set - right? Should I also add that to the TLSSocket call in tlsConnectionListener?

@r1b
Copy link
Contributor Author

r1b commented Sep 20, 2019

Sorry, this needs more work

@r1b r1b closed this Sep 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants