Buffer.from not support SharedArrayBuffer

[wlunlimited]

create a buffer from SharedArrayBuffer just like the buff.from sample in doc:

let sab =new SharedArrayBuffer(24);
const arr = new Uint16Array(sab);

arr[0] = 5000;
arr[1] = 4000;

// Shares memory with arr
const buf = Buffer.from(arr.buffer);

// Prints: <Buffer 88 13 a0 0f>
console.log(buf);

// Changing the original Uint16Array changes the Buffer also
arr[1] = 6000;

// Prints: <Buffer 88 13 70 17>
console.log(buf);

• Version:
• Platform:
• Subsystem:

run above code :
node --harmony-sharedarraybuffer ./test.js
and then we would got a error below:

buffer.js:259
throw new TypeError(kFromErrorMsg);
^

TypeError: First argument must be a string, Buffer, ArrayBuffer, Array, or array-like object.
at fromObject (buffer.js:259:9)
at Function.Buffer.from (buffer.js:96:10)
at Object. (/home/Myprojects/node_git/test_buff_sab.js:8:20)
at Module._compile (module.js:556:32)
at Object.Module._extensions..js (module.js:565:10)
at Module.load (module.js:473:32)
at tryModuleLoad (module.js:432:12)
at Function.Module._load (module.js:424:3)
at Module.runMain (module.js:590:10)
at run (bootstrap_node.js:394:7)

[addaleax]

I’ve labelled this good first contribution and would be happy to help anyone who wants to have a go at this!

[jasnell]

Good catch on this. I'd been meaning to give this a try but hadn't gotten around to it.
/cc @nodejs/buffer

[vkurchatkin]

Isn't it a bit early for this?

[jasnell]

Perhaps, but we've taken fixes for other early bits to make sure things work. For example, the SIMD stuff in util. I wouldn't say that it's a priority but it's worth looking into.

[trevnorris]

The fix is simple. Add a instanceof SharedArrayBuffer check. But, without the flag there's no global. Are we supposed to do a if (!global.SharedArrayBuffer) SharedArrayBuffer = null or some such before it's exposed by default?

[ojss]

I would like to help with this issue.

[addaleax]

@ojss Awesome! I would maybe wait for #8453 to be merged, or start from there, so that the changes there don’t conflict with yours. That PR should also give a few hints as to where changes are necessary. :)

A couple of hints: You’ll probably need to add one or two lines to src/node_util.cc for SharedArrayBuffer detection – if you’ve never done anything with C++ before, don’t worry, the changes should be straightforward. Since this feature requires you to run node with a special flag, namely with --harmony-sharedarraybuffer, you can take a look at the top of test/parallel/test-util-inspect-simd.js to see how to integrate that into the tests.

If you have any questions, feel free to ask here or in #node-dev on Freenode. :)

[ojss]

@addaleax Thank you! I will most certainly requiring help as I still don't completely understand the internals of node.
I saw the PR, it did give me an idea of what I should be doing. But I still don't understand what I must do in node_util.cc. Just add a check to see of the flag is enabled and accordingly set a global?

[addaleax]

@ojss There’s this list of things that provide the JS side with helpers such as isArrayBuffer(), so that it’s easier to check whether something is an ArrayBuffer or not (see here for one example usage). That list probably needs an isSharedArrayBuffer entry. :)

[ojss]

@addaleax Yes I had assumed as much. But what I don't get is where are those functions defined?
The macro assigns a name to a function that is exposed on the JS side right?Am I misunderstanding something?
Is there some documentation that I can refer to?
:)

[addaleax]

The macro assigns a name to a function that is exposed on the JS side right?

Yeah, that’s what it does. Using macros for that is a bit of magic, but it keeps the code short and makes it easy to change the list.

If you’re asking where these functions pop up – that’s the line at the top of buffer.js which is (currently) const { isArrayBuffer } = process.binding('util');, where you may need to add isSharedArrayBuffer, too.

Is there some documentation that I can refer to?

Not sure what you mean – there aren’t any docs for internal stuff like that in node_util.cc right now…

[ojss]

Well looks like I got a few things correct.

Yeah, that’s what it does. Using macros for that is a bit of magic, but it keeps the code short and makes it easy to change the list.

I am sorry but I still don't understand where the definition of the function is? Is it generated on the fly?

[addaleax]

I am sorry but I still don't understand where the definition of the function is? Is it generated on the fly?

Okay, so here is how that whole thing works (I hope I’m not overexplaining but in any case there’s a good chance other people are reading this who would love to know more about the C++ side of node themselves):

#define VALUE_METHOD_MAP(V)                                                   \
  V(isArrayBuffer, IsArrayBuffer)                                             \
  V(isDataView, IsDataView)                                                   \
  …

That #define VALUE_METHOD_MAP(V) defines a macro which takes another macro as its parameter V so that that other macro gets replaced whenever V is encountered. For example, in the block following the definition of VALUE_METHOD_MAP:

#define V(_, ucname) \
  static void ucname(const FunctionCallbackInfo<Value>& args) {               \
    CHECK_EQ(1, args.Length());                                               \
    args.GetReturnValue().Set(args[0]->ucname());                             \
  }

  VALUE_METHOD_MAP(V)
#undef V

Here one concrete V is defined; It ignores its first parameter (which would be e.g. isArrayBuffer) and labels its second parameter ucname (which would be e.g. IsArrayBuffer – uc stands for upper case here, as opposed to isArrayBuffer).

When that V is expanded, a C++ function is generated that follows a format suitable for calling it from JS, that’s the void ucname(const FunctionCallbackInfo<Value>& args) { part. In its body, it makes sure that there is exactly one parameter to that function, and sets the return value to args[0]->ucname() – so, for example, it calls args[0]->IsArrayBuffer() and just passes the return value along.

The VALUE_METHOD_MAP(V) part does the magic here – when VALUE_METHOD_MAP is expanded, it generates a list of Vs, and all of those Vs are expanded into C++ functions, each as they were just defined. (The #undef V is just cleanup so that another V can be defined later without the compiler complaining.)

Now, scroll scroll scroll, near the bottom of the file there’s an Initialize function which contains, among other things, this:

#define V(lcname, ucname) env->SetMethod(target, #lcname, ucname);
  VALUE_METHOD_MAP(V)
#undef V

So, again, VALUE_METHOD_MAP gets expanded into many Vs, and each of these Vs generates something like env->SetMethod(target, "isArrayBuffer", IsArrayBuffer) – it attaches the IsArrayBuffer C++ function that has been defined above to some object as isArrayBuffer. That’s the object you get when you call process.binding('util') in JS! That’s undocumented, though, because it’s internal and people shouldn’t rely on how things work exactly.

[wlunlimited]

the most simple patch is like below(for node 6.5.0),but it is necessary to run node with --harmony-sharedarraybuffer :

diff --git a/lib/buffer.js b/lib/buffer.js
index 8489ca0..849e223 100644
--- a/lib/buffer.js
+++ b/lib/buffer.js
@@ -251,6 +251,10 @@ function fromObject(obj) {
       return fromArrayLike(obj);
     }

+    if (obj instanceof SharedArrayBuffer) {
+      return new FastBuffer(obj);
+    }
+
     if (obj.type === 'Buffer' && Array.isArray(obj.data)) {
       return fromArrayLike(obj.data);
     }

Since SharedArrayBuffer is not supported by default now,above code would throw exception if run node without --harmony-sharedarraybuffer.

[wlunlimited]

so,we maybe could do like this:

diff --git a/lib/buffer.js b/lib/buffer.js
index 8489ca0..138047f 100644
--- a/lib/buffer.js
+++ b/lib/buffer.js
@@ -251,6 +251,11 @@ function fromObject(obj) {
       return fromArrayLike(obj);
     }

+    if (SharedArrayBuffer != undefined && SharedArrayBuffer != null) {
+      if (obj instanceof SharedArrayBuffer) {
+        return new FastBuffer(obj);
+      }
+    }
     if (obj.type === 'Buffer' && Array.isArray(obj.data)) {
       return fromArrayLike(obj.data);
     }

@addaleax @ojss it is work with --harmony-sharedarraybuffer or without this option.What do you think？

[ojss]

@wlunlimited thats probably the way to go :)

if (SharedArrayBuffer != undefined && SharedArrayBuffer != null) {
if (isSharedArrayBuffer(obj.buffer)) {
return new FastBuffer(obj);
}
}

@addaleax wow that is very interesting. If you don't mind I have one more question.
What is the significance of this: https://github.com/nodejs/node/blob/master/deps/v8/src/js/macros.py
Is it somehow related to the expansions?

please check if this the right way to go about it: ojss@836186a

[addaleax]

What is the significance of this: https://github.com/nodejs/node/blob/master/deps/v8/src/js/macros.py Is it somehow related to the expansions?

That has nothing to do with it. :) The #define macro expansion is the C preprocessor’s job, that is a typical part of the compiling process for C/C++.

SharedArrayBuffer != undefined && SharedArrayBuffer != null

That’s only necessary if you use SharedArrayBuffer directly. It might be best if you try and keep the code changes similar to the locations where there ArrayBuffer support is already there.

please check if this the right way to go about it: ojss/node@836186a

I see nothing wrong with that so far – you’ll want to update the rest of buffer.js and add some tests. (We typically want commit messages <= 50 characters in length, btw. If you’re unsure about something like that, you can have a look at the contributing.md.)

Because this requires a flag you’ll probably want to put it into a separate test file – here are some tipps for that, although you can probably have looks at the other Buffer tests for guidance.

@wlunlimited One reason we’re using isArrayBuffer already (and even more so after #8453) is that Node has the ability to run JS code in different “contexts”, which each have their own set of language builtins. For example, this doesn’t work as expected:

> vm.runInNewContext('new ArrayBuffer') instanceof ArrayBuffer
false

[ojss]

@addaleax @wlunlimited
The above commit now allows SharedArrayBuffer to be used with Buffer.from.
Just need to write the tests now.

[ojss]

Is the below test fine?

/*global SharedArrayBuffer*/
/*eslint no-undef: "error"*/
'use strict';
// Flags: --harmony-sharedarraybuffer

require('../common');
const assert = require('assert');
const Buffer = require('buffer').Buffer;

const sab = new SharedArrayBuffer(24);
const arr = new Uint16Array(sab);
const ar = new Uint16Array(12);
ar[0] = 5000;
arr[0] = 5000;
arr[1] = 4000;
ar[1] = 4000;

var arr_buf = Buffer.from(arr.buffer);
var ar_buf = Buffer.from(ar.buffer);

assert.strictEqual(Buffer.compare(arr_buf, ar_buf), 0, 'SAB == ARBuf');

arr[1] = 6000;
ar[1] = 6000;

assert.strictEqual(Buffer.compare(arr_buf, ar_buf), 0);

[addaleax]

@ojss The Flags: line should probably come first in the test file, you can use assert.deepStrictEqual(buf1, buf2) instead of an extra Buffer.compare, and you may want to add tests for Buffer.byteLength, which should also accept SharedArrayBuffers.

But generally, yeah, it looks like you’re ready for opening a pull request – it’s much easier to comment on specific lines, run the tests, etc. that way.

Maybe one more thing: If you want to propose a change that closes an issue like this one, you can add a line like Fixes: https://github.com/nodejs/node/issues/8440 to your git commit message :)

[wlunlimited]

@addaleax @ojss sorry,the patch above is just a demo, it is not completed yet.Because it is not handle the length of Buffer.from(arraybuffer,length).

let sab =new SharedArrayBuffer(24);
const arr = new Uint16Array(sab);

arr[0] = 5000;
arr[1] = 4000;

// Shares memory with arr
const buf = Buffer.from(arr.buffer,10);

this would ignore the length 10 and make buffer from the whole arr.buffer.

[ojss]

@wlunlimited If you see the definition of Buffer.from, you will notice that at the end there is a call to fromObject(). fromObject only takes the object and not the length. So to account for the length we would have to change the signature of fromObject.

or I could make another function similar to fromArrayBuffer, in fact maybe even get fromArrayBuffer to handle the SharedArrayBuffer? Is that possible @addaleax?

I will be opening a pull request in some time.