search

ibmruntimes / yieldable-json

Asynchronous JSON parser and stringify APIs that make use of generator patterns
Other
145 stars 22 forks source link

Protect object for stringifyAsync ? #27

Closed logidelic closed 4 years ago

logidelic commented 4 years ago

Since stringifyAsync async, I assume that the Javascript object passed to it must be protected: i.e. caller must ensure that it does not get modified until stringifyAsync calls back. Is that right?

If the caller accidentally does not protect it, and it happens to get modified before stringifyAsync is finished its thing, what are the repercussions? Will stringifyAsync somehow handle this gracefully?

I feel like this is pretty critical and should be documented or else it will bite a lot of people...

gireeshpunathil commented 4 years ago

@logidelic - thanks for bringing this up. yes, the repercussion of modification is inadvertent crash / object corruption. I am just wondering what is the standard practice of documenting in such cases. I don't remember any precedence on this in node.js documentation or elsewhere. Do you have a reference to share?

logidelic commented 4 years ago

I don't think there's a "standard" because I don't think that any other typical node module (and certainly nothing that's part of node) can crash just because the user modified an object used during an async op...

I just meant that the user should be warned in the stringifyAsync documentation. Something like what you said in your reply:

Warning: While stringifyAsync is in progress (i.e. before the callback is executed), it is the user's responsibility to ensure that the Javascript object value (or any of its child objects) is not modified in any way. The repercussion of modification is undefined behavior (and can possibly result in an inadvertent crash or object corruption).

gireeshpunathil commented 4 years ago

looking back, I have made it clear when I wrote about the APIs (caveat section in https://developer.ibm.com/node/2017/11/13/yieldable-json-asynchronous-json-module-node-js/ ) but may be, I felt it was not necessary here. Or may be, something else, I forgot!

feel free to come up with a PR if you like, or else np, I will!