Fixes an issue where Buffer.from was not supporting SharedArrayBuffer by ojss · Pull Request #8510 · nodejs/node

ojss · 2016-09-13T03:07:50Z

Checklist

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

Affected core subsystem(s)

buffer

Description of change

Exposes isSharedArrayBuffer to check for SharedArrayBuffer type object in JS.
Buffer.from can now take a SharedArrayBuffer as an argument.
refer to issue #8440.

Trott · 2016-09-13T03:34:21Z

test/parallel/test-buffer-sharedarraybuffer.js

This is already set in the main project .eslintrc file so this comment should be unnecessary, unless there's something I'm missing.

The second line isn't necessary but the first one is absolutely required or else the linter fails.

add value method map to check if given object is a SharedArrayBuffer.

add an isSharedBuffer check to Buffer.from() fromArrayBuffer() can now handle a SharedArrayBuffer object. Fixes an issue where Buffer.from was not supporting SharedArrayBuffer Fixes: nodejs#8440

refer issue nodejs#8440

mscdex · 2016-09-13T06:19:25Z

CI: https://ci.nodejs.org/job/node-test-pull-request/4020/

ojss · 2016-09-13T06:21:58Z

@mscdex I will be force pushing a squashed set of commits is that alright?

mscdex · 2016-09-13T06:41:49Z

@ojss Yes

addaleax · 2016-09-13T12:36:10Z

test/parallel/test-buffer-sharedarraybuffer.js

+ar[1] = 4000;
+
+var arr_buf = Buffer.from(arr.buffer);
+var ar_buf = Buffer.from(ar.buffer);


I think these can be const, too

addaleax · 2016-09-13T12:39:02Z

Hi @ojss, this looks great so far! There are a couple of other places in lib/buffer.js where isArrayBuffer() is used… Could you take a look at these, too?

ojss · 2016-09-13T14:29:13Z

@addaleax namely these areas I assume:
https://github.com/nodejs/node/blob/master/lib/buffer.js#L267-L273
and here:
https://github.com/nodejs/node/blob/master/lib/buffer.js#L352-L356

Will do. :)

addaleax · 2016-09-13T14:30:23Z

@ojss exactly, thanks!

Updated test-buffer-sharedarraybuffer.js to have more readable variable names. Replaced var definitions with const.

ojss · 2016-09-13T15:02:24Z

@addaleax please have a look.

mscdex · 2016-09-13T15:06:48Z

lib/buffer.js

Although I prefer this style as well, this project's style places the operators on the right-hand side:

if (isArrayBuffer(obj.buffer) || 'length' in obj || isSharedArrayBuffer(obj)) {

You might also move the second condition to a new line as well to make the code easier to scan visually.

Added a isSharedArrayBuffer check in Buffer.byteLength method. Added a isSharedArrayBuffer check in fromObject method. Buffer.byteLength can potentially benefit from this change as it can be used interchangeably with the byteLength property of SharedArrayBuffer.

The new assertion checks of the output of Buffer.byteLength() is equal to the SharedArrayBuffer property byteLength.

ojss · 2016-09-13T15:55:39Z

lib/buffer.js


  if (obj) {
-    if (isArrayBuffer(obj.buffer) || 'length' in obj) {
+    if (isArrayBuffer(obj.buffer) || 'length' in obj ||


@mscdex changed the style here.

mscdex · 2016-09-13T20:30:51Z

CI again: https://ci.nodejs.org/job/node-test-pull-request/4034/

ojss · 2016-09-14T11:49:33Z

@mscdex @addaleax why is that test pending for so long?

mscdex · 2016-09-14T16:10:55Z

@ojss Just CI infrastructure issues.

LGTM.

jasnell

LGTM

addaleax

LGTM

ojss · 2016-09-17T07:50:23Z

@addaleax @mscdex by when will the pull request to be accepted?

mscdex · 2016-09-17T08:53:42Z

Landed in 2a2ec9d. Thanks @ojss!

Fixes: #8440 PR-URL: #8510 Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

ojss · 2016-09-17T09:06:54Z

Thanks for the help @addaleax and @mscdex!

addaleax · 2016-09-17T12:46:31Z

Awesome, great to have you on board! It’s quite usual that it takes a couple of days until PRs are landed, but if you feel like something is ready to land and it has been a while, pinging is exactly the right thing to do.

addaleax · 2016-09-23T14:02:13Z

lib/buffer.js

  if (obj) {
-    if (isArrayBuffer(obj.buffer) || 'length' in obj) {
+    if (isArrayBuffer(obj.buffer) || 'length' in obj ||
+        isSharedArrayBuffer(obj)) {


@ojss Should this have been obj.buffer?

@addaleax you're right! I am so sorry about this mistake, I will fix this right away.

Yeah, sorry I didn’t notice that during review myself! :/

while we're at it can we do something about 'length' in obj? by "do something" i mean let's just simplify it for !!obj.length or some such. using in here is wasting a lot of performance.

wait. this already landed. okay, ill take care of it another time.

MylesBorins · 2016-10-06T22:23:13Z

@mscdex @addaleax should this be backported?

addaleax · 2016-10-06T22:34:46Z

@thealphanerd yeah… no… maybe? Is there some kind of succinct description of what commits should or should not go into LTS branches that one could judge commits by? I think I’ve heard something like “changes that increase stability”, and I wouldn’t count this as such.

It’s really not that important for v4, it only adds more support for a feature that’s still behind a flag.

MylesBorins · 2016-10-06T22:44:20Z

@addaleax that was exactly what I wanted to know, if it is behind a flag then it shouldn't land

Thanks

isSharedArrayBuffer in fromObject was missing obj.buffer moved the 'length' in obj check so that it is checked first making the code slightly more performant and able to handle SharedArrayBuffer without relying on an explicit check. Ref: #8510 PR-URL: #8739 Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Fixes: #8440 PR-URL: #8510 Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

isSharedArrayBuffer in fromObject was missing obj.buffer moved the 'length' in obj check so that it is checked first making the code slightly more performant and able to handle SharedArrayBuffer without relying on an explicit check. Ref: #8510 PR-URL: #8739 Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. labels Sep 13, 2016

Trott reviewed Sep 13, 2016
View reviewed changes

ojss force-pushed the 8440-fix branch from 571eede to e8fd414 Compare September 13, 2016 05:07

buffer: add isSharedArrayBuffer to node_util.cc and buffer.js

699b605

add value method map to check if given object is a SharedArrayBuffer.

ojss force-pushed the 8440-fix branch from e8fd414 to c844b95 Compare September 13, 2016 05:33

ojss added 2 commits September 13, 2016 11:46

buffer: add isSharedArrayBuffer checks to buffer.js

7a1e765

add an isSharedBuffer check to Buffer.from() fromArrayBuffer() can now handle a SharedArrayBuffer object. Fixes an issue where Buffer.from was not supporting SharedArrayBuffer Fixes: nodejs#8440

test: add tests for SharedArrayBuffer

ff3555f

refer issue nodejs#8440

ojss force-pushed the 8440-fix branch from 67ba26e to ff3555f Compare September 13, 2016 06:44

addaleax reviewed Sep 13, 2016
View reviewed changes

tests: update test-buffer-sharedarraybuffer

ce9aa0b

Updated test-buffer-sharedarraybuffer.js to have more readable variable names. Replaced var definitions with const.

mscdex reviewed Sep 13, 2016
View reviewed changes

ojss added 2 commits September 13, 2016 21:22

test: update sharedarraybuffer test to reflect changes to byteLength()

bdfaa52

The new assertion checks of the output of Buffer.byteLength() is equal to the SharedArrayBuffer property byteLength.

ojss force-pushed the 8440-fix branch from 7c446b4 to bdfaa52 Compare September 13, 2016 15:53

ojss reviewed Sep 13, 2016
View reviewed changes

jasnell approved these changes Sep 15, 2016

View reviewed changes

addaleax approved these changes Sep 15, 2016

View reviewed changes

mscdex closed this Sep 17, 2016

mscdex pushed a commit that referenced this pull request Sep 17, 2016

buffer: add isSharedArrayBuffer checks

2a2ec9d

Fixes: #8440 PR-URL: #8510 Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

ojss deleted the 8440-fix branch September 17, 2016 09:20

addaleax mentioned this pull request Sep 17, 2016

util: Add format for SharedArrayBuffer #8587

Closed

3 tasks

addaleax reviewed Sep 23, 2016

View reviewed changes

ojss mentioned this pull request Sep 23, 2016

buffer: fix check of buffer and rearrange conditional #8739

Closed

4 tasks

MylesBorins added the lts-watch-v4.x label Oct 6, 2016

MylesBorins added dont-land-on-v4.x and removed lts-watch-v4.x labels Oct 6, 2016

Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016

buffer: add isSharedArrayBuffer checks

ee319b7

Fixes: #8440 PR-URL: #8510 Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

ThomasdenH mentioned this pull request Sep 20, 2017

[types/node] Change Buffer.from to accept SharedArrayBuffer DefinitelyTyped/DefinitelyTyped#19656

Closed

Uh oh!

Conversation

ojss commented Sep 13, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex commented Sep 13, 2016

Uh oh!

ojss commented Sep 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mscdex commented Sep 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax commented Sep 13, 2016

Uh oh!

ojss commented Sep 13, 2016

Uh oh!

addaleax commented Sep 13, 2016

Uh oh!

ojss commented Sep 13, 2016

Uh oh!

mscdex Sep 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex commented Sep 13, 2016

Uh oh!

ojss commented Sep 14, 2016

Uh oh!

mscdex commented Sep 14, 2016

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

ojss commented Sep 17, 2016

Uh oh!

mscdex commented Sep 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ojss commented Sep 17, 2016

Uh oh!

addaleax commented Sep 17, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MylesBorins commented Oct 6, 2016

Uh oh!

addaleax commented Oct 6, 2016

Uh oh!

MylesBorins commented Oct 6, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

ojss commented Sep 13, 2016 •

edited

Loading

mscdex Sep 13, 2016 •

edited

Loading

mscdex commented Sep 17, 2016 •

edited

Loading