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.

Sign up for GitHub

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

Jump to bottom

[v8.x backport] tls: accept array of protocols in TLSSocket #21721

Closed
wants to merge 1 commit into from
Closed

[v8.x backport] tls: accept array of protocols in TLSSocket #21721

wants to merge 1 commit into from

Conversation

BethGriggs
Copy link
Member

Brings the ALPNProtocols & NPNProtocols options of TLSSocket in line
with the documentation. i.e. an array of strings for protocols may be
used, not only a buffer.

PR-URL: #16655
Fixes: https://github.com/node/issues/16643
Reviewed-By: Ben Noordhuis info@bnoordhuis.nl
Reviewed-By: James M Snell jasnell@gmail.com
Reviewed-By: Anatoli Papirovski apapirovski@mac.com

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

Backport, as requested, of #16655

@nodejs-github-bot
Copy link
Collaborator

@BethGriggs build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/230/pipeline

@nodejs-github-bot nodejs-github-bot added tls Issues and PRs related to the tls subsystem. v8.x labels Jul 9, 2018
@BethGriggs BethGriggs added the wip Issues and PRs that are still a work in progress. label Jul 9, 2018
@qubyte @BethGriggs
tls: accept array of protocols in TLSSocket
07f3d85 
Brings the ALPNProtocols & NPNProtocols options of TLSSocket in line
with the documentation. i.e. an array of strings for protocols may be
used, not only a buffer.

PR-URL: nodejs#16655
Fixes: https://github.com/node/issues/16643
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
lpinca
lpinca reviewed Jul 9, 2018
View reviewed changes
lib/_tls_wrap.js
this._tlsOptions = {};
else
this._tlsOptions = options;
function TLSSocket(socket, opts) {

This comment was marked as resolved.

Sign in to view

lib/_tls_wrap.js
else
this._tlsOptions = options;
function TLSSocket(socket, opts) {
const tlsOptions = Object.assign({}, opts);

This comment was marked as resolved.

Sign in to view

@lpinca
Copy link
Member

lpinca commented Jul 9, 2018

Ignore me I didn't notice it was a backport, sorry.

@BethGriggs BethGriggs removed the wip Issues and PRs that are still a work in progress. label Jul 10, 2018
@MylesBorins
Copy link
Contributor

One more CI https://ci.nodejs.org/job/node-test-pull-request/16098/

@MylesBorins
Copy link
Contributor

Windows failures

15:23:04 not ok 390 parallel/test-timers-throw-reschedule
15:23:04   ---
15:23:04   duration_ms: 0.401
15:23:04   severity: fail
15:23:04   stack: |-
15:23:04     assert.js:42
15:23:04       throw new errors.AssertionError({
15:23:04       ^
15:23:04     
15:23:04     AssertionError [ERR_ASSERTION]: false == true
15:23:04         at Timeout.common.mustCall (c:\workspace\node-test-binary-windows\test\parallel\test-timers-throw-reschedule.js:20:5)
15:23:04         at Timeout._onTimeout (c:\workspace\node-test-binary-windows\test\common\index.js:478:15)
15:23:04         at ontimeout (timers.js:498:11)
15:23:04         at Timer.unrefdHandle (timers.js:611:5)

MylesBorins pushed a commit that referenced this pull request Aug 1, 2018
@qubyte @MylesBorins
tls: accept array of protocols in TLSSocket
d99b665 
Brings the ALPNProtocols & NPNProtocols options of TLSSocket in line
with the documentation. i.e. an array of strings for protocols may be
used, not only a buffer.

Backport-PR-URL: #21721
PR-URL: #16655
Fixes: https://github.com/node/issues/16643
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@MylesBorins
Copy link
Contributor

That test ended up being a known flake now fixed on v8.x-staging

landed in d99b665

@MylesBorins MylesBorins closed this Aug 1, 2018
rvagg pushed a commit that referenced this pull request Aug 16, 2018
@qubyte @rvagg
tls: accept array of protocols in TLSSocket
1cf17df 
Brings the ALPNProtocols & NPNProtocols options of TLSSocket in line
with the documentation. i.e. an array of strings for protocols may be
used, not only a buffer.

Backport-PR-URL: #21721
PR-URL: #16655
Fixes: https://github.com/node/issues/16643
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@BethGriggs BethGriggs deleted the backport-16655-to-v8.x branch June 11, 2019 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpinca lpinca lpinca left review comments

@addaleax addaleax addaleax approved these changes

Assignees
No one assigned
Labels
tls Issues and PRs related to the tls subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@BethGriggs @nodejs-github-bot @lpinca @MylesBorins @addaleax @qubyte