We need a version of `napi_buffer_new()` where a deleter is given

[gabrielschulhof]

node::Buffer::New() has a version where it takes a char * along with the size. Node assumes ownership of the memory at the char * and frees it using free() when the buffer is garbage collected. It also provides a version of New() where a deleter may be specified in case free() is not the right thing to do upon GC.

To avoid excessive copying, addons should be able to set the deleter to a no-op. To that end we need a version of napi_buffer_new() that accepts a deleter. I propose

napi_status napi_buffer_new_with_deleter(napi_env e,
                                         char* data,
                                         size_t size,
                                         void (*deleter)(void*),
                                         napi_value* result);

[gabrielschulhof]

Forgot the context ...

napi_status napi_buffer_new_with_deleter(napi_env e,
                                         char* data,
                                         size_t size,
                                         void (*deleter)(char*, void*),
                                         void* deleter_context,
                                         napi_value* result);

[jasongin]

I agree, it seems like a good API to add. There are a few details to consider.

• The buffer APIs are currently named inconsistently with everything else, e.g. napi_buffer_new instead of napi_create_buffer.
• It should probably use napi_finalize to be consistent with the deleter callbacks used by napi_wrap and napi_create_external. napi_finalize doesn't currently take a context, aside from the pointer that is to be deleted. Does it need a context?
• There is a similar API already for creating arraybuffers on external data napi_create_external_arraybuffer however it is missing a deleter callback, that it probably should have for the same reasons.

[gabrielschulhof]

Well, node::Buffer deleters take a context, but I guess we needn't expose that.

…

On Mon, Feb 20, 2017 at 8:24 PM, Jason Ginchereau ***@***.***> wrote: I agree, it seems like a good API to add. There are a few details to consider. - The buffer APIs are currently named inconsistently with everything else, e.g. napi_buffer_new instead of napi_create_buffer. - It should probably use napi_finalize to be consistent with the deleter callbacks used by napi_wrap and napi_create_external. napi_finalize doesn't currently take a context, aside from the pointer that is to be deleted. Does it need a context? - There is a similar API already for creating arraybuffers on external data napi_create_external_arraybuffer however it is missing a deleter callback, that it probably should have for the same reasons. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#108 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA7k0Ya9PtA63YMWCpSaJw0rLWvkFP9Qks5redpQgaJpZM4MGLW1> .

[gabrielschulhof]

TBH, I'm not sure we need napi_buffer_new() at all. In fact, we should have

napi_status napi_create_buffer(napi_env e,
                               char* data,
                               size_t size,
                               napi_finalize finalize_cb,
                               napi_value* result);

and no other variation on napi_create_buffer(). We can document that finalize_cb may be null, in which case free() will be called on the pointer upon gc.

[jasongin]

We can document that finalize_cb may be null, in which case free() will be called on the pointer upon gc.

I'm concerned that wouldn't be obvious, so I'd prefer 2 separate functions. (I commented on this in the PR.) Also for the case of allocating a new buffer, it would be simpler to let node manage allocating and freeing the memory.

[jasongin]

The ChakraCore port of updated buffer APIs is ready in my fork. It depends on the Reference and external improvements, so I'll wait for that to land before submitting the PR.

The concerns I had about the buffer value type and the null isolate turned out to be not a problem, since the methods that are called on the isolate happen to succeed even when it is null. I did have to modify the buffer test script to do GCs a tick after discarding the objects, due to different behavior of the Chakra GC.