External data lifetime for Buffer and ArrayBuffer

[kfarnung]

While writing up the documentation for Buffer and ArrayBuffer I came across some suspicious looking factory methods:

static ArrayBuffer New(napi_env env, void* externalData, size_t byteLength);
static Buffer<T> New(napi_env env, T* data, size_t length);

Digging through the code a bit, it appears that these methods take in a pointer to data and then create an external wrapper object with no finalizer.

Looking at the NAN documentation for buffers it seems like the behavior is to take ownership of the memory and then later free it. Even NAN's approach is arguably dangerous since it makes assumptions about how and where the memory was allocated.

Since node-addon-api doesn't actually free the data and doesn't provide any way for the user to free the data, the only use case I can see for this API is for data which has a static lifetime.

Given their limited (and dangerous) usage, maybe we should consider removing these overloads?

[kfarnung]

Per discussion today investigate mimicking the NAN behavior.

[github-actions]

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.

[mhdawson]

@NickNaso will review and lead the discussion in a future team meeting.

[mhdawson]

Touched on in the meeting today, no update, but @NickNaso still looking at it.

[NickNaso]

Hi everybody, I did not have the time to work on issue until today. I'm starting work on this now, I will update at next Node-API meeting.

[gabrielschulhof]

@NickNaso will modify the Buffer and ArrayBuffer documentation to add a warning for the ::New() variants that do not accept a finalizer about the fact that Buffers and ArrayBuffers created in such a way will not provide the caller with an opportunity to free the data when they get garbage-collected.