Skip to content

tls: support Uint8Arrays for protocol list buffers#11984

Closed
addaleax wants to merge 1 commit intonodejs:masterfrom
addaleax:tls-uint8array
Closed

tls: support Uint8Arrays for protocol list buffers#11984
addaleax wants to merge 1 commit intonodejs:masterfrom
addaleax:tls-uint8array

Conversation

@addaleax
Copy link
Member

@addaleax addaleax commented Mar 22, 2017

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
Affected core subsystem(s)

@addaleax addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 22, 2017
@nodejs-github-bot nodejs-github-bot added the tls Issues and PRs related to the tls subsystem. label Mar 22, 2017
@mscdex
Copy link
Contributor

mscdex commented Mar 22, 2017

Why is this one semver-minor when the others are labeled as semver-major?

@jasnell
Copy link
Member

jasnell commented Mar 22, 2017

@mscdex .. the other PRs had error message changes, this one does not.

lpinca
lpinca reviewed Mar 23, 2017
View reviewed changes
test/parallel/test-tls-basic-validations.js Outdated
Copy link
Member

@lpinca lpinca Mar 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Why using the spread operator? If I'm not wrong passing a buffer to the Uint8Array constructor (new Uint8Array(Buffer.from('abcd'))) works.

Copy link
Member

@TimothyGu TimothyGu Mar 23, 2017

Choose a reason for hiding this comment

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

For this case, indeed it would work. Passing a Buffer to Uint8Array makes the Uint8Array use the identical underlying ArrayBuffer, which at times may not be desirable. But in this case we are creating a new Buffer every time so it should be fine.

Copy link
Member

@lpinca lpinca Mar 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@TimothyGu AFAIK memory is copied to the TypedArray not shared.

Copy link
Member

@TimothyGu TimothyGu Mar 23, 2017

Choose a reason for hiding this comment

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

@lpinca, heh I was mistaken. In that case, yeah there's no reason to use spread syntax...

Copy link
Member Author

@addaleax addaleax Mar 24, 2017

Choose a reason for hiding this comment

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

@lpinca @TimothyGu right, thanks for pointing out. fixed!

@addaleax
Copy link
Member Author

addaleax commented Mar 27, 2017

Landed in c3efe72

@addaleax addaleax closed this Mar 27, 2017
@addaleax addaleax deleted the tls-uint8array branch March 27, 2017 00:11
addaleax added a commit that referenced this pull request Mar 27, 2017
@addaleax
tls: support Uint8Arrays for protocol list buffers
c3efe72 
PR-URL: #11984
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins
Copy link
Contributor

MylesBorins commented Mar 28, 2017

This needs to be manually backported to v7.x

@jasnell jasnell mentioned this pull request Apr 4, 2017
Closed
@gibfahn gibfahn mentioned this pull request Jun 2, 2017
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@jasnell jasnell jasnell approved these changes

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig approved these changes

+1 more reviewer

@TimothyGu TimothyGu TimothyGu left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@addaleax @mscdex @jasnell @MylesBorins @bnoordhuis @lpinca @TimothyGu @cjihrig @nodejs-github-bot