doc: ArrayBuffer and Buffer documentation #256
Conversation
kfarnung
commented
May 2, 2018
kfarnung
commented
- Also fixed a typo in the draft message
doc/buffer.md
Outdated
| valid for the lifetime of the Buffer object. Since the Buffer is subject to | ||
| garbage collection this overload is only suitable for data which is static and | ||
| never needs to be freed. |
There was a problem hiding this comment.
Can someone fact-check this? I did some digging in the code and this appears to be true.
If so, it's a departure from NAN which would take ownership of the data and later free it. Something to keep in mind for the conversion script, if someone was depending on this behavior there would be a memory leak in the converted code.
There was a problem hiding this comment.
I opened #258 to discuss
kfarnung
changed the title
|
|
||
| Wraps the provided external data into a new `ArrayBuffer` object. | ||
|
|
||
| The `ArrayBuffer` object does not assume ownership for the data and expects it |
There was a problem hiding this comment.
It is not more accurate that it effectively does take ownership, taking responsibility to run the finalizer to provided to delete the data when the class is deleted?
There was a problem hiding this comment.
I toyed with the wording here a bit, but since the module code is still responsible for cleanup I feel like it's more accurate to say that ownership is never transferred. The caller is merely lending the data to the ArrayBuffer. Doing a bit more reading, it seems to align pretty well with the way V8 describes these "externalized" ArrayBuffer objects.
After ArrayBuffer had been externalized, it does no longer own the memory block. The caller should take steps to free memory when it is no longer needed.
There was a problem hiding this comment.
@kfarnung did we close on what we wanted to do? It would be good to get this landed.
| The `ArrayBuffer` object does not assume ownership for the data and expects it | ||
| to be valid for the lifetime of the object. Since the `ArrayBuffer` is subject | ||
| to garbage collection this overload is only suitable for data which is static | ||
| and never needs to be freed. |
There was a problem hiding this comment.
I like your description here.
I know we discussed doing a free, but thinking about it I don't like us trying to free an object that we don't know was allocated with malloc or new.
What would be good to figure out though is how to avoid conversion errors from NaN which result in leaks. I'd be tempted to remove or deprecate this one and force callers to indicate they don't need finalization by passing a null to one of the other methods. Since the wrapper is not part of the abi contract I think we could delete the method and provide instructions on what to use instead.
There was a problem hiding this comment.
Maybe just deprecate as opposed to remove with the doc saying "don't use"
There was a problem hiding this comment.
My only concern about "doc deprecated" is that it requires people to see the doc when converting their code.
|
@kfarnung can you take a pass through to update to what you think we should do for now. Would be good to get an initial version out and then we can iterate. |
|
Will do, I was on vacation for a week and I'm just getting back up to speed. |
doc/buffer.md
Outdated
|
|
||
| Wraps the provided external data into a new `Buffer` object. | ||
|
|
||
| The Bu`Buffer`ffer object does not assume ownership for the data and expects it |
There was a problem hiding this comment.
There's a typo here.
doc/buffer.md
Outdated
|
|
||
| Allocates a new `Buffer` object and copies the provided external data into it. | ||
|
|
||
| The `Buffer` object does not assume ownership for the data and expects it to be |
There was a problem hiding this comment.
I don't get it. If it copies the data, how does it not assume ownership of it?
There was a problem hiding this comment.
The Buffer doesn't assume ownership for the data you're providing to the Copy call, it copies it to memory that it does own. This was supposed to contrast with the other Buffer factory functions, but maybe it's not necessary?
PR-URL: #256 Reviewed-By: Michael Dawson <[email protected]>
|
Landed as 11697fc |
PR-URL: nodejs/node-addon-api#256 Reviewed-By: Michael Dawson <[email protected]>
PR-URL: nodejs/node-addon-api#256 Reviewed-By: Michael Dawson <[email protected]>
PR-URL: nodejs/node-addon-api#256 Reviewed-By: Michael Dawson <[email protected]>
PR-URL: nodejs/node-addon-api#256 Reviewed-By: Michael Dawson <[email protected]>
