Protecting ArrayBuffers from the whatwg reference implementation

[trxcllnt]

The whatwg ReadableByteStreamController reference implementation attempts to mimic the ArrayBuffer.transfer semantics by copying the underlying ArrayBuffer of any TypedArray that passes through it, and redefining the original's byteLength to be 0:

// Not implemented correctly
exports.TransferArrayBuffer = O => {
  assert(!exports.IsDetachedBuffer(O));
  const transferredIshVersion = O.slice();

  // This is specifically to fool tests that test "is transferred" by taking a non-zero-length
  // ArrayBuffer and checking if its byteLength starts returning 0.
  Object.defineProperty(O, 'byteLength', {
    get() {
      return 0;
    }
  });
  O[isFakeDetached] = true;

  return transferredIshVersion;
};

To be fair, they state that the reference implementation shouldn't be used as a polyfill. But seeing as how it is being used as a polyfill, this behavior is problematic.

Obviously copying at every step is unacceptable for a production polyfill, but redefining byteLength also makes it impossible to use in node (without wrapping every stream to pre-copy each chunk) for interop or testing DOM-streams logic. Node often uses internal ArrayBuffer pools (like here in fs.ReadStream), so the Object.defineProperty call is modifying the ArrayBuffer pool, breaking unrelated operations dependent on that pool.

After thoroughly reading/stepping through the code paths in the reference implementation in each place TransferArrayBuffer is called to verify the returned buffer isn't modified further, and verifying the behavior with about a thousand (auto-generated) tests and one production use, I've come up with a workaround that solves both these issues:

const kIsFakeBuffer = Symbol.for('isFakeBuffer');
function protectArrayBufferFromWhatwgRefImpl(value: Uint8Array) {
    const real = value.buffer;
    if (!real[kIsFakeBuffer]) {
        const fake = Object.create(real);
        Object.defineProperty(fake, kIsFakeBuffer, { value: true });
        Object.defineProperty(fake, 'slice', { value: () => real });
        Object.defineProperty(value, 'buffer', {     value: fake });
    }
    return value;
}

Before calling readableByteStreamController.enqueue(), we can redefine the buffer property on the TypedArray to be an instance created via Object.create(), whose "slice" function is overridden to return the original, unmodified buffer. This ensure the value returned to transferredIshVersion = O.slice(); will be the "real" underlying ArrayBuffer, and won't be sliced.

I could keep this hack in userland, but maybe adding something like this to the polyfill would be worthwhile for the following reasons:

1. Every userland call to controller.enqueue() has to be guarded by a call to protectArrayBufferFromWhatwgRefImpl(), which makes writing a true zero-copy polyfill for FF that much harder.
2. Keeping this fix in userland means that we have to call protectArrayBufferFromWhatwgRefImpl() even in environments where the web-streams-polyfill isn't being used, like Chrome and Safari. Even though in my testing this wrapping doesn't affect the native stream implementations, if for some reason a user did want to call chunk.buffer.slice(), they'd be getting the real ArrayBuffer out, not a slice of it.
3. My fulltime job is optimizing a binary streaming engine across node + browsers, and it took me a while to track this down -- what chance does a regular dev just trying to stream some data cross-browser stand of discovering and fixing this issue on their own?

[MattiasBuelens]

Indeed, TransferArrayBuffer aims to replicate the side effects of transferring an ArrayBuffer, but in order to do so it copies the ArrayBuffer - which is exactly what you want to avoid with a transfer! This is fine for the purposes of the reference implementation, but unacceptable for a polyfill.

I think I'll replace TransferArrayBuffer and IsDetachedBuffer with dummy implementations in the polyfill. All WPT tests related to transferring buffers are in readable-byte-streams/detached-buffers.any.js, so I can just exclude that one file in the polyfill's test suite.

---

Node often uses internal ArrayBuffer pools (like here in fs.ReadStream), so the Object.defineProperty call is modifying the ArrayBuffer pool, breaking unrelated operations dependent on that pool.

Although we can fix this for the polyfill, I think this can still cause issues in the future when using a native readable byte stream implementation. A native implementation would actually transfer the ArrayBuffer, so any references to the old buffer become "neutered" and cannot be re-used.

In your next function, you pass the Uint8Array returned by reader.read() to controller.enqueue(). The enqueue method transfers the underlying buffer, so that buffer can never be used for a future call to reader.read(). If I understand your question correctly, you want these buffers to come from a pool. This can become a problem if two calls to reader.read() are allowed to return two Uint8Array views on the same ArrayBuffer from that pool.

The "intended" way to do zero-copy reads with a readable byte stream is to use controller.byobRequest instead of controller.enqueue(). Therefore, you must have your reader use a given buffer to store the read data in, rather than having your reader return a (new or re-used) buffer. For example, assuming you had a reader.readInto(view: ArrayBufferView): Promise<number> method that returns the number of bytes read into view, you could wrap a readable byte stream around that:

const readable = new ReadableStream({
  type: 'bytes',
  async pull(controller) {
    const byobRequest = controller.byobRequest;
    if (byobRequest) {
      // using a BYOB reader
      const bytesRead = await reader.readInto(byobRequest.view);
      if (bytesRead === 0) {
        controller.close();
      } else {
        byobRequest.respond(bytesRead);
      }
    } else {
      // using a default reader
      const buf = await reader.read();
      if (!buf) {
        controller.close();
      } else {
        controller.enqueue(buf.slice()); // need to copy if `buf.buffer` could be re-used
      }
    }
  }
});

Alternatively, you can set autoAllocateChunkSize so you always have a byobRequest even when a default reader is used. That way, you only need the if block in the above pull() method.

---

Some off-topic remarks:

• You shouldn't need both start and pull methods if both simply do await next(controller). You can leave out the start method.
• In your next function, you don't need a loop. If the internal queue size (as indicated by controller.desiredSize) is greater than 0, the stream implementation will automatically call your pull method again. 😉

[MattiasBuelens]

The "intended" way to do zero-copy reads with a readable byte stream is to use controller.byobRequest instead of controller.enqueue().

Unfortunately, for this to be truly zero-copy, you would need to use BYOB readers everywhere, which would require a massive rewrite... I guess that's not really an option. 😕

Perhaps an easier option would be to always copy the chunk before enqueuing it:

async function next(controller: ReadableStreamDefaultController<Uint8Array>) {
  let buf = await reader.read(controller.desiredSize || null);
  if (buf) {
    controller.enqueue(buf.slice());
  } else {
    controller.close();
  }
}

That way, you don't have to change the rest of the code, at the cost of one copy per chunk (given that the polyfill is fixed to not copy in TransferArrayBuffer). However, it seems like this copy is unavoidable: even Node's file reader copies the read bytes to avoid consumers messing with the pool.

[trxcllnt]

I think I'll replace TransferArrayBuffer and IsDetachedBuffer with dummy implementations in the polyfill. All WPT tests related to transferring buffers are in readable-byte-streams/detached-buffers.any.js, so I can just exclude that one file in the polyfill's test suite.

That's an even better fix! I tested that locally and it worked, but assumed failing the wpt tests was a non-starter, so didn't bother suggesting it.

In your next function, you pass the Uint8Array returned by reader.read() to controller.enqueue(). The enqueue method transfers the underlying buffer, so that buffer can never be used for a future call to reader.read(). If I understand your question correctly, you want these buffers to come from a pool. This can become a problem if two calls to reader.read() are allowed to return two Uint8Array views on the same ArrayBuffer from that pool.

Yes, that's the heart of the issue. I'm happy to conform to the semantics of whatever APIs I need to get the job done, so in my mind it's not so much about what I want (aside from ensuring zero-copy), and more about making the polyfill play nice with node.

Although we can fix this for the polyfill, I think this can still cause issues in the future when using a native readable byte stream implementation. A native implementation would actually transfer the ArrayBuffer, so any references to the old buffer become "neutered" and cannot be re-used.

Yes, but at the moment the only pooling that's happening is in node core, so if node ever gets a native ReadableByteStream implementation, they'll have to deal with this issue at that point anyway.

even Node's file reader copies the read bytes to avoid consumers messing with the pool

In that method thisPool is a node Buffer, where slice() is the same as TypedArray's subarray(), so it doesn't incur a copy. Confusing, I know 😕.

The "intended" way to do zero-copy reads with a readable byte stream is to use controller.byobRequest instead of controller.enqueue(). Therefore, you must have your reader use a given buffer to store the read data in, rather than having your reader return a (new or re-used) buffer. For example, assuming you had a reader.readInto(view: ArrayBufferView): Promise method that returns the number of bytes read into view, you could wrap a readable byte stream around that:

Yes, I actually do this a few steps before in AdaptiveByteReader#read(size?). Allow me to clarify a bit, because the code I've linked to in this issue is... special.

The Apache Arrow project is a streaming zero-copy dataframe standard for large analytics workloads. We compile many flavors of the JS library for both node and browsers, and we want to provide a simple and consistent public API for reading/writing data regardless of which environment you're in. We're sensitive to bundle size, and would like to use the native APIs in each environment, so we accept and produce the environment's stream primitives at the edges, and adapt them to Iterables and AsyncIterables for use internally.

We have a few generic io primitive impls, but they're intentionally slim wrappers that adapt the native io primitives to our Iterable/AsyncIterable protocol. We could split everything out and build special versions for each environment, but doing this means there's just one npm package to consume, maximizes code reuse (I'm the main JS contributor at the moment, and I'm stretched pretty thin), and reduces the surface area/complexity for future maintainers.

The "intended" way to do zero-copy reads with a readable byte stream is to use controller.byobRequest instead of controller.enqueue().

Unfortunately, for this to be truly zero-copy, you would need to use BYOB readers everywhere, which would require a massive rewrite... I guess that's not really an option. 😕

Perhaps an easier option would be to always copy the chunk before enqueuing it:

Yep, and this is where we reach the limits of our ability to cleanly bridge all these APIs. The "lowest-common-denominator" concept in all these APIs is buffer message-passing. I could bake the "byob" concept into our "bridge" primitives, but then I force a copy in node, and am ultimately just rebuilding whatwg reference impl. At the end of the day, forcing a copy anywhere (node or the browser) means we can't do things like this as flawlessly:

Thanks for considering this change!

Best,
Paul

[MattiasBuelens]

That's an even better fix! I tested that locally and it worked, but assumed failing the wpt tests was a non-starter, so didn't bother suggesting it.

In most cases I would agree, but in this case there's no way to have an efficient implementation that passes the tests. For the tests to pass, you have to make the original ArrayBuffer appear "neutered", and there's just no "good" way to do that.

I'm hoping the ArrayBuffer.prototype.transfer() proposal goes through, so we can provide a correct and efficient implementation on modern browsers. Until then, I'm okay with breaking these couple of tests.

In that method thisPool is a node Buffer, where slice() is the same as TypedArray's subarray(), so it doesn't incur a copy. Confusing, I know 😕.

Wow, TIL that Buffer.slice != TypedArray.slice! That's a very unfortunate naming conflict. 😝

If I understand correctly, a Node ReadStream creates an ArrayBuffer as its "pool", and uses that to fulfill multiple reads. Once the pool is full, it allocates a new one and uses that for future reads. And when all references to the original pool are gone (because all references to the pushed chunks are gone), the original ArrayBuffer gets garbage collected.

Yeah, it would be cool if we could do that with readable byte streams... You could almost get it to work with:

outer: while (true) {
  let pool = new ArrayBuffer(1024);
  let offset = 0;
  while (offset < pool.byteLength) {
    const { done, value } = await reader.read(new Uint8Array(pool, offset));
    if (done) {
      break outer;
    }
    pool = value.buffer;
    offset += value.byteLength;
    this.push(value);
  }
}

But then the next call to reader.read() would invalidate all previously pushed chunks using that same pool. Unless you can somehow guarantee that those previous chunks are no longer needed when the next chunk is read... but that's probably not possible. 😞

Yep, and this is where we reach the limits of our ability to cleanly bridge all these APIs. The "lowest-common-denominator" concept in all these APIs is buffer message-passing.

I see. Indeed, that's quite a challenge to get a common I/O API across all platforms.

At the end of the day, forcing a copy anywhere (node or the browser) means we can't do things like this as flawlessly:

That looks amazing! Good luck with the project! 👍