facebook/hermes

Support for ArrayBuffer::isMutableBuffer() and ArrayBuffer::getMutableBuffer()

mrousavy opened this issue · 5 comments

mrousavy commented

Problem

In Nitro, array buffers are supported types for native modules.

Imagine the following native nitro module:

const seedArrayBuffer = myNitroModule.createRandomSeed()
const keyString = myNitroModule.getEncryptionKey(seedArrayBuffer)

On the native side of getEncryptionKey, we can safely access the ArrayBuffer's data without any problems just fine because it is synchronous and we are running on the JS thread:

auto arrayBuffer = args[0].getObject(runtime).getArrayBuffer(runtime);
void* data = arrayBuffer.data(runtime);

..but as soon as we make this function asynchronous:

const seedArrayBuffer = myNitroModule.createRandomSeed()
const keyString = await myNitroModule.getEncryptionKey(seedArrayBuffer)

We can no longer safely access the ArrayBuffer's data because we are running on a separate Thread.

Nitro will convert the jsi::ArrayBuffer to a custom Nitro type that basically tells you to not access it on a different Thread and will throw if you try to do so. So the user is always forced to make a copy of the data before switching to a different Thread.

I think it'd be a cool API to get access to the underlying std::shared_ptr<jsi::MutableBuffer> if it has one (not every ArrayBuffer is created with one):

auto arrayBuffer = args[0].getObject(runtime).getArrayBuffer(runtime);
auto mutableBuffer = arrayBuffer.getMutableBuffer(runtime);
std::thread([=]() {
  void* data = mutableBuffer.data();
});

Note: This could cause data race issues if not used correctly. I think it's up to the user to guard against that (either via Mutexes, or just praying at night that their APIs will not be misused)

Solution

  • jsi::ArrayBuffer::isMutableBuffer(..)
  • jsi::ArrayBuffer::getMutableBuffer(..)
  • jsi::ArrayBuffer::asMutableBuffer(..) (?)

Additional Context

In Nitro, I currently solved it like this:

If I receive an ArrayBuffer from JS, it is currently always a nitro::JSArrayBuffer. I would love to look into it and unwrap the jsi::MutableBuffer from it so I can optionally also pass the user a nitro::NativeArrayBuffer instead though.

tmikov commented

The team discussed this and we think it is a good idea. Thanks for proposing it! It is now in the short term TODO list.

  • ❤️1
mrousavy commented

Thank you! 🙏

mrousavy commented

Question; does every ArrayBuffer have a jsi::MutableBuffer, even those created from JS?

Or does this only apply to ArrayBuffers that were created with the jsi::MutableBuffer API?

tmikov commented

Question; does every ArrayBuffer have a jsi::MutableBuffer, even those created from JS?
Or does this only apply to ArrayBuffers that were created with the jsi::MutableBuffer API?

Technically, today no ArrayBuffers have a jsi::MutableBuffer, since there is no way to obtain it back. But I get what you are asking. When we add ArrayBuffer::getMutableBuffer, will it work on any ArrayBuffer?

I don't think we envisioned that it would, but at the same time I think that it could work.

Do you have a motivating use case?

mrousavy commented

Do you have a motivating use case?

Well, there's a nice use-case: mrousavy/nitro#601

I cannot access .data() on the jsi::ArrayBuffer from a different Thread, so I'd HAVE to copy it. (yes synchronisation is the user's responsibility)
If it was a jsi::MutableBuffer, it'd be safer I guess.

  • 👍1