napi: revamp buffer API

* There are now three creation functions: 1. Create such that node owns the pointer 2. Create such that the caller owns the pointer 3. Create such that node owns the pointer and copies the data into it * napi_buffer_has_instance() is now called napi_is_buffer() * napi_buffer_data() and napi_buffer_length() are now merged into napi_get_buffer_info() * There is now a test for this api under test/addons-abi Fixes nodejs/abi-stable-node#108 Closes nodejs/abi-stable-node#109
boingoing · Feb 22, 2017 · 5042fc7 · 5042fc7
1 parent 654da16
commit 5042fc7
Show file tree

Hide file tree

Showing 26 changed files with 252 additions and 65 deletions.
diff --git a/src/node_jsvmapi.cc b/src/node_jsvmapi.cc
@@ -1951,52 +1951,74 @@ napi_status napi_get_and_clear_last_exception(napi_env e, napi_value* result) {
   return napi_ok;
 }
 
-napi_status napi_buffer_new(napi_env e, char* data, size_t size, napi_value* result) {
+napi_status napi_create_buffer(napi_env e, size_t size, char** data,
+                               napi_value* result) {
   NAPI_PREAMBLE(e);
+  CHECK_ARG(data);
   CHECK_ARG(result);
 
-  auto maybe = node::Buffer::New(v8impl::V8IsolateFromJsEnv(e), data, size);
+  auto maybe = node::Buffer::New(v8impl::V8IsolateFromJsEnv(e), size);
 
   CHECK_MAYBE_EMPTY(maybe, napi_generic_failure);
 
-  *result = v8impl::JsValueFromV8LocalValue(maybe.ToLocalChecked());
+  v8::Local<v8::Object> jsBuffer = maybe.ToLocalChecked();
+
+  *result = v8impl::JsValueFromV8LocalValue(jsBuffer);
+  *data = node::Buffer::Data(jsBuffer);
+
   return GET_RETURN_STATUS();
 }
 
-napi_status napi_buffer_copy(napi_env e, const char* data,
-                             size_t size, napi_value* result) {
+napi_status napi_create_external_buffer(napi_env e, size_t size, char* data,
+                                        napi_finalize finalize_cb,
+                                        napi_value* result) {
   NAPI_PREAMBLE(e);
   CHECK_ARG(result);
 
-  auto maybe = node::Buffer::Copy(v8impl::V8IsolateFromJsEnv(e), data, size);
+  auto maybe = node::Buffer::New(v8impl::V8IsolateFromJsEnv(e), data, size,
+    (node::Buffer::FreeCallback)finalize_cb, nullptr);
 
   CHECK_MAYBE_EMPTY(maybe, napi_generic_failure);
 
   *result = v8impl::JsValueFromV8LocalValue(maybe.ToLocalChecked());
   return GET_RETURN_STATUS();
 }
 
-napi_status napi_buffer_has_instance(napi_env e, napi_value v, bool* result) {
+napi_status napi_create_buffer_copy(napi_env e, const char* data,
+                                    size_t size, napi_value* result) {
   NAPI_PREAMBLE(e);
   CHECK_ARG(result);
 
-  *result = node::Buffer::HasInstance(v8impl::V8LocalValueFromJsValue(v));
+  auto maybe = node::Buffer::Copy(v8impl::V8IsolateFromJsEnv(e), data, size);
+
+  CHECK_MAYBE_EMPTY(maybe, napi_generic_failure);
+
+  *result = v8impl::JsValueFromV8LocalValue(maybe.ToLocalChecked());
   return GET_RETURN_STATUS();
 }
 
-napi_status napi_buffer_data(napi_env e, napi_value v, char** result) {
+napi_status napi_is_buffer(napi_env e, napi_value v, bool* result) {
   NAPI_PREAMBLE(e);
   CHECK_ARG(result);
 
-  *result = node::Buffer::Data(v8impl::V8LocalValueFromJsValue(v).As<v8::Object>());
+  *result = node::Buffer::HasInstance(v8impl::V8LocalValueFromJsValue(v));
   return GET_RETURN_STATUS();
 }
 
-napi_status napi_buffer_length(napi_env e, napi_value v, size_t* result) {
+napi_status napi_get_buffer_info(napi_env e, napi_value v, char** data,
+                                 size_t* length) {
   NAPI_PREAMBLE(e);
-  CHECK_ARG(result);
 
-  *result = node::Buffer::Length(v8impl::V8LocalValueFromJsValue(v));
+  v8::Local<v8::Object> buffer =
+    v8impl::V8LocalValueFromJsValue(v).As<v8::Object>();
+
+  if (data) {
+    *data = node::Buffer::Data(buffer);
+  }
+  if (length) {
+    *length = node::Buffer::Length(buffer);
+  }
+
   return GET_RETURN_STATUS();
 }
 

diff --git a/src/node_jsvmapi.h b/src/node_jsvmapi.h
@@ -315,17 +315,21 @@ NODE_EXTERN napi_status napi_is_exception_pending(napi_env e, bool* result);
 NODE_EXTERN napi_status napi_get_and_clear_last_exception(napi_env e, napi_value* result);
 
 // Methods to provide node::Buffer functionality with napi types
-NODE_EXTERN napi_status napi_buffer_new(napi_env e,
-                                        char* data,
-                                        size_t size,
-                                        napi_value* result);
-NODE_EXTERN napi_status napi_buffer_copy(napi_env e,
-                                         const char* data,
-                                         size_t size,
-                                         napi_value* result);
-NODE_EXTERN napi_status napi_buffer_has_instance(napi_env e, napi_value v, bool* result);
-NODE_EXTERN napi_status napi_buffer_data(napi_env e, napi_value v, char** result);
-NODE_EXTERN napi_status napi_buffer_length(napi_env e, napi_value v, size_t* result);
+NODE_EXTERN napi_status napi_create_buffer(napi_env e,
+                                           size_t size,
+                                           char** data,
+                                           napi_value* result);
+NODE_EXTERN napi_status napi_create_external_buffer(napi_env e,
+                                                    size_t size,
+                                                    char* data,
+                                                    napi_finalize finalize_cb,
+                                                    napi_value* result);
+NODE_EXTERN napi_status napi_create_buffer_copy(napi_env e,
+                                                const char* data,
+                                                size_t size,
+                                                napi_value* result);
+NODE_EXTERN napi_status napi_is_buffer(napi_env e, napi_value v, bool* result);
+NODE_EXTERN napi_status napi_get_buffer_info(napi_env e, napi_value v, char** data, size_t* length);
 
 // Methods to work with array buffers and typed arrays
 NODE_EXTERN napi_status napi_is_arraybuffer(napi_env env, napi_value value, bool* result);

diff --git a/test/addons-abi/1_hello_world/test.js b/test/addons-abi/1_hello_world/test.js
@@ -1,6 +1,6 @@
 'use strict';
-require('../../common');
+var common = require('../../common');
 var assert = require('assert');
-var addon = require('./build/Release/binding');
+var addon = require(`./build/${common.buildType}/binding`);
 
 assert.equal(addon.hello(), 'world');
diff --git a/test/addons-abi/2_function_arguments/test.js b/test/addons-abi/2_function_arguments/test.js
@@ -1,6 +1,6 @@
 'use strict';
-require('../../common');
+var common = require('../../common');
 var assert = require('assert');
-var addon = require('./build/Release/binding');
+var addon = require(`./build/${common.buildType}/binding`);
 
 assert.equal(addon.add(3, 5), 8);
diff --git a/test/addons-abi/3_callbacks/test.js b/test/addons-abi/3_callbacks/test.js
@@ -1,7 +1,7 @@
 'use strict';
-require('../../common');
+var common = require('../../common');
 var assert = require('assert');
-var addon = require('./build/Release/binding');
+var addon = require(`./build/${common.buildType}/binding`);
 
 addon.RunCallback(function(msg) {
   assert.equal(msg, 'hello world');

diff --git a/test/addons-abi/4_object_factory/test.js b/test/addons-abi/4_object_factory/test.js
@@ -1,7 +1,7 @@
 'use strict';
-require('../../common');
+var common = require('../../common');
 var assert = require('assert');
-var addon = require('./build/Release/binding');
+var addon = require(`./build/${common.buildType}/binding`);
 
 var obj1 = addon('hello');
 var obj2 = addon('world');

diff --git a/test/addons-abi/5_function_factory/test.js b/test/addons-abi/5_function_factory/test.js
@@ -1,7 +1,7 @@
 'use strict';
-require('../../common');
+var common = require('../../common');
 var assert = require('assert');
-var addon = require('./build/Release/binding');
+var addon = require(`./build/${common.buildType}/binding`);
 
 var fn = addon();
 assert.equal(fn(), 'hello world'); // 'hello world'
diff --git a/test/addons-abi/6_object_wrap/test.js b/test/addons-abi/6_object_wrap/test.js
@@ -1,7 +1,7 @@
 'use strict';
-require('../../common');
+var common = require('../../common');
 var assert = require('assert');
-var addon = require('./build/Release/binding');
+var addon = require(`./build/${common.buildType}/binding`);
 
 var obj = new addon.MyObject(9);
 assert.equal(obj.value, 9);

diff --git a/test/addons-abi/7_factory_wrap/test.js b/test/addons-abi/7_factory_wrap/test.js
@@ -1,7 +1,7 @@
 'use strict';
-require('../../common');
+var common = require('../../common');
 var assert = require('assert');
-var createObject = require('./build/Release/binding');
+var createObject = require(`./build/${common.buildType}/binding`);
 
 var obj = createObject(10);
 assert.equal(obj.plusOne(), 11);

diff --git a/test/addons-abi/8_passing_wrapped/test.js b/test/addons-abi/8_passing_wrapped/test.js
@@ -1,7 +1,7 @@
 'use strict';
-require('../../common');
+var common = require('../../common');
 var assert = require('assert');
-var addon = require('./build/Release/binding');
+var addon = require(`./build/${common.buildType}/binding`);
 
 var obj1 = addon.createObject(10);
 var obj2 = addon.createObject(20);

diff --git a/test/addons-abi/test_array/test.js b/test/addons-abi/test_array/test.js
@@ -1,9 +1,9 @@
 'use strict';
-require('../../common');
+var common = require('../../common');
 var assert = require('assert');
 
 // Testing api calls for arrays
-var test_array = require('./build/Release/test_array');
+var test_array = require(`./build/${common.buildType}/test_array`);
 
 var array = [
   1,

diff --git a/test/addons-abi/test_buffer/binding.gyp b/test/addons-abi/test_buffer/binding.gyp
@@ -0,0 +1,8 @@
+{
+  "targets": [
+    {
+      "target_name": "test_buffer",
+      "sources": [ "test_buffer.cc" ]
+    }
+  ]
+}
diff --git a/test/addons-abi/test_buffer/test.js b/test/addons-abi/test_buffer/test.js
@@ -0,0 +1,24 @@
+'use strict';
+// Flags: --expose-gc
+
+var common = require('../../common');
+var binding = require(`./build/${common.buildType}/test_buffer`);
+var assert = require( "assert" );
+
+assert( binding.newBuffer().toString() === binding.theText,
+  "buffer returned by newBuffer() has wrong contents" );
+assert( binding.newExternalBuffer().toString() === binding.theText,
+  "buffer returned by newExternalBuffer() has wrong contents" );
+console.log( "gc1" );
+global.gc();
+assert( binding.getDeleterCallCount(), 1, "deleter was not called" );
+assert( binding.copyBuffer().toString() === binding.theText,
+  "buffer returned by copyBuffer() has wrong contents" );
+
+var buffer = binding.staticBuffer();
+assert( binding.bufferHasInstance( buffer ), true, "buffer type checking fails" );
+assert( binding.bufferInfo( buffer ), true, "buffer data is accurate" );
+buffer = null;
+global.gc();
+console.log( "gc2" );
+assert( binding.getDeleterCallCount(), 2, "deleter was not called" );
diff --git a/test/addons-abi/test_buffer/test_buffer.cc b/test/addons-abi/test_buffer/test_buffer.cc
@@ -0,0 +1,128 @@
+#include <string.h>
+#include <string>
+#include <node_api_helpers.h>
+
+#define JS_ASSERT(env, assertion, message) \
+  if (!(assertion)) { \
+    napi_throw_error((env), (std::string("assertion (" #assertion ") failed: ") + message).c_str()); \
+    return; \
+  }
+
+#define NAPI_CALL(env, theCall) \
+  if (theCall != napi_ok) { \
+    const char *errorMessage = napi_get_last_error_info()->error_message; \
+    errorMessage = errorMessage ? errorMessage: "empty error message"; \
+    napi_throw_error((env), errorMessage); \
+    return; \
+  }
+
+static const char theText[] =
+  "Lorem ipsum dolor sit amet, consectetur adipiscing elit.";
+
+static int deleterCallCount = 0;
+static void deleteTheText(void *data) {
+  delete (char *)data;
+  deleterCallCount++;
+}
+
+static void noopDeleter(void *data) {
+  deleterCallCount++;
+}
+
+NAPI_METHOD(newBuffer) {
+  napi_value theBuffer;
+  char *theCopy;
+  NAPI_CALL(env, napi_create_buffer(env, sizeof(theText), &theCopy,
+    &theBuffer));
+  JS_ASSERT(env, theCopy, "Failed to copy static text for newBuffer");
+  strcpy(theCopy, theText);
+  NAPI_CALL(env, napi_set_return_value(env, info, theBuffer));
+}
+
+NAPI_METHOD(newExternalBuffer) {
+  napi_value theBuffer;
+  char *theCopy = strdup(theText);
+  JS_ASSERT(env, theCopy,
+    "Failed to copy static text for newExternalBuffer");
+  NAPI_CALL(env, napi_create_external_buffer(env, sizeof(theText), theCopy,
+    deleteTheText, &theBuffer));
+  NAPI_CALL(env, napi_set_return_value(env, info, theBuffer));
+}
+
+NAPI_METHOD(getDeleterCallCount) {
+  napi_value callCount;
+  NAPI_CALL(env, napi_create_number(env, deleterCallCount, &callCount));
+  NAPI_CALL(env, napi_set_return_value(env, info, callCount));
+}
+
+NAPI_METHOD(copyBuffer) {
+  napi_value theBuffer;
+  NAPI_CALL(env, napi_create_buffer_copy(env, theText, sizeof(theText),
+    &theBuffer));
+  NAPI_CALL(env, napi_set_return_value(env, info, theBuffer));
+}
+
+NAPI_METHOD(bufferHasInstance) {
+  int argc;
+  NAPI_CALL(env, napi_get_cb_args_length(env, info, &argc));
+  JS_ASSERT(env, argc == 1, "Wrong number of arguments");
+  napi_value theBuffer;
+  NAPI_CALL(env, napi_get_cb_args(env, info, &theBuffer, 1));
+  bool hasInstance;
+  napi_valuetype theType;
+  NAPI_CALL(env, napi_get_type_of_value(env, theBuffer, &theType));
+  JS_ASSERT(env, theType == napi_object,
+    "bufferHasInstance: instance is not an object");
+  NAPI_CALL(env, napi_is_buffer(env, theBuffer, &hasInstance));
+  JS_ASSERT(env, hasInstance, "bufferHasInstance: instance is not a buffer");
+  napi_value returnValue;
+  NAPI_CALL(env, napi_create_boolean(env, hasInstance, &returnValue));
+  NAPI_CALL(env, napi_set_return_value(env, info, returnValue));
+}
+
+NAPI_METHOD(bufferInfo) {
+  int argc;
+  NAPI_CALL(env, napi_get_cb_args_length(env, info, &argc));
+  JS_ASSERT(env, argc == 1, "Wrong number of arguments");
+  napi_value theBuffer, returnValue;
+  NAPI_CALL(env, napi_get_cb_args(env, info, &theBuffer, 1));
+  char *bufferData;
+  size_t bufferLength;
+  NAPI_CALL(env, napi_get_buffer_info(env, theBuffer, &bufferData,
+    &bufferLength));
+  NAPI_CALL(env, napi_create_boolean(env,
+    !strcmp(bufferData, theText) && bufferLength == sizeof(theText),
+    &returnValue));
+  NAPI_CALL(env, napi_set_return_value(env, info, returnValue));
+}
+
+NAPI_METHOD(staticBuffer) {
+  napi_value theBuffer;
+  NAPI_CALL(env, napi_create_external_buffer(env, sizeof(theText),
+    (char *)theText, noopDeleter, &theBuffer));
+  NAPI_CALL(env, napi_set_return_value(env, info, theBuffer));
+}
+
+void Init(napi_env env, napi_value exports, napi_value module) {
+  napi_propertyname propName;
+  napi_value theValue;
+
+  NAPI_CALL(env, napi_property_name(env, "theText", &propName));
+  NAPI_CALL(env, napi_create_string_utf8(env, theText, sizeof(theText), &theValue));
+  NAPI_CALL(env, napi_set_property(env, exports, propName, theValue));
+
+  napi_property_descriptor methods[] = {
+    { "newBuffer", newBuffer },
+    { "newExternalBuffer", newExternalBuffer },
+    { "getDeleterCallCount", getDeleterCallCount },
+    { "copyBuffer", copyBuffer },
+    { "bufferHasInstance", bufferHasInstance },
+    { "bufferInfo", bufferInfo },
+    { "staticBuffer", staticBuffer }
+  };
+
+  NAPI_CALL(env, napi_define_properties(env, exports,
+    sizeof(methods)/sizeof(methods[0]), methods));
+}
+
+NODE_MODULE_ABI(addon, Init)
diff --git a/test/addons-abi/test_constructor/test.js b/test/addons-abi/test_constructor/test.js
@@ -1,9 +1,9 @@
 'use strict';
-require('../../common');
+var common = require('../../common');
 var assert = require('assert');
 
 // Testing api calls for a constructor that defines properties
-var TestConstructor = require('./build/Release/test_constructor');
+var TestConstructor = require(`./build/${common.buildType}/test_constructor`);
 var test_object = new TestConstructor();
 
 assert.equal(test_object.echo('hello'), 'hello');

diff --git a/test/addons-abi/test_exception/test.js b/test/addons-abi/test_exception/test.js
@@ -1,6 +1,7 @@
 'use strict';
 
-var test_exception = require( "./build/Release/test_exception" );
+var common = require('../../common');
+var test_exception = require(`./build/${common.buildType}/test_exception`);
 var assert = require( "assert" );
 var theError = new Error( "Some error" );
 var throwTheError = function() {