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 option #29635

Closed
wants to merge 3 commits into from
Closed

tls: honor pauseOnConnect option #29635

wants to merge 3 commits into from

Conversation

r1b
Copy link
Contributor

@r1b r1b commented Sep 20, 2019

pauseOnConnect is now passed along to the net.Socket constructor from
the tls.Socket constructor. The readable flag must match the value of
pauseOnConnect. Tests were added to cover all available net.Server
options when used in the tls.Server constructor.

Fixes: #29620
Refs: #27665

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

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or

(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or

(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.

(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.

@nodejs-github-bot nodejs-github-bot added the tls Issues and PRs related to the tls subsystem. label Sep 20, 2019
lib/_tls_wrap.js Show resolved Hide resolved
lib/_tls_wrap.js Outdated Show resolved Hide resolved
`pauseOnConnect` is now passed along to the net.Socket constructor from
the tls.Socket constructor. The `readable` flag must match the value of
`pauseOnConnect`. Tests were added to cover all available net.Server
options when used in the tls.Server constructor.

Fixes: #29620
Refs: #27665
@r1b r1b requested review from mscdex and lpinca September 20, 2019 22:31
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

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

SGTM.

Trott
Trott previously approved these changes Sep 24, 2019
@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 24, 2019
@Trott Trott dismissed their stale review September 24, 2019 06:03

I'm a little uncertain about pauseOnCreate vs. pauseOnConnect and should defer to subject-matter experts anyway.

@Trott
Copy link
Member

Trott commented Sep 25, 2019

@nodejs/collaborators This could use another review.

@jasnell
Copy link
Member

jasnell commented Sep 25, 2019

Are documentation updates needed for this?

Co-Authored-By: Rich Trott <[email protected]>
@r1b
Copy link
Contributor Author

r1b commented Sep 25, 2019

@jasnell Currently the tls.createServer docs indicate that:

Any net.createServer() option can be provided.

This will be correct once this is merged. Currently only allowHalfOpen has any effect.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@jasnell jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Sep 30, 2019
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 3, 2019

@Trott
Copy link
Member

Trott commented Oct 4, 2019

Landed in 1e12859

@Trott Trott closed this Oct 4, 2019
Trott pushed a commit that referenced this pull request Oct 4, 2019
`pauseOnConnect` is now passed along to the net.Socket constructor from
the tls.Socket constructor. The `readable` flag must match the value of
`pauseOnConnect`. Tests were added to cover all available net.Server
options when used in the tls.Server constructor.

Fixes: #29620
Refs: #27665

PR-URL: #29635
Reviewed-By: Luigi Pinca <[email protected]>
BridgeAR pushed a commit that referenced this pull request Oct 9, 2019
`pauseOnConnect` is now passed along to the net.Socket constructor from
the tls.Socket constructor. The `readable` flag must match the value of
`pauseOnConnect`. Tests were added to cover all available net.Server
options when used in the tls.Server constructor.

Fixes: #29620
Refs: #27665

PR-URL: #29635
Reviewed-By: Luigi Pinca <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Oct 10, 2019
BridgeAR added a commit that referenced this pull request Oct 10, 2019
Notable changes:

* build:
  * Add `--force-context-aware` flag to prevent usage of native node
    addons that aren't context aware
    #29631
* deprecations:
  * Add documentation-only deprecation for `process._tickCallback()`
    #29781
* esm:
  * Using JSON modules is experimental again
    #29754
* fs:
  * Introduce `opendir()` and `fs.Dir` to iterate through directories
    #29349
* process:
  * Add source-map support to stack traces by using
    `--source-map-support` #29564
* tls:
  * Honor `pauseOnConnect` option
    #29635
  * Add option for private keys for OpenSSL engines
    #28973

PR-URL: #29919
BridgeAR added a commit that referenced this pull request Oct 11, 2019
Notable changes:

* build:
  * Add `--force-context-aware` flag to prevent usage of native node
    addons that aren't context aware
    #29631
* deprecations:
  * Add documentation-only deprecation for `process._tickCallback()`
    #29781
* esm:
  * Using JSON modules is experimental again
    #29754
* fs:
  * Introduce `opendir()` and `fs.Dir` to iterate through directories
    #29349
* process:
  * Add source-map support to stack traces by using
    `--source-map-support` #29564
* tls:
  * Honor `pauseOnConnect` option
    #29635
  * Add option for private keys for OpenSSL engines
    #28973

PR-URL: #29919
BridgeAR added a commit that referenced this pull request Oct 11, 2019
Notable changes:

* build:
  * Add `--force-context-aware` flag to prevent usage of native node
    addons that aren't context aware
    #29631
* deprecations:
  * Add documentation-only deprecation for `process._tickCallback()`
    #29781
* esm:
  * Using JSON modules is experimental again
    #29754
* fs:
  * Introduce `opendir()` and `fs.Dir` to iterate through directories
    #29349
* process:
  * Add source-map support to stack traces by using
    `--source-map-support` #29564
* tls:
  * Honor `pauseOnConnect` option
    #29635
  * Add option for private keys for OpenSSL engines
    #28973

PR-URL: #29919
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. semver-minor PRs that contain new features and should be released in the next minor version. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tls: pauseOnConnect option not honored
6 participants