Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove ArrayPtr<byte> from list of supported jsg wrapping types #79

Merged
merged 1 commit into from Oct 10, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 0 additions & 1 deletion src/workerd/jsg/jsg.h
Expand Up @@ -223,7 +223,6 @@ class JsExceptionThrown: public std::exception {
// - C++ kj::OneOf<T, U, ...> <-> JS T or U or ...
// - C++ kj::Array<T> <-> JS Array of T
// - C++ kj::Array<byte> <-> JS ArrayBuffer
// - C++ kj::ArrayPtr<byte> <- JS ArrayBuffer, ArrayBufferView
// - C++ jsg::Dict<T> <-> JS Object used as a map of strings to values of type T
// - C++ jsg::Function<T(U, V, ...)> <-> JS Function
// - C++ jsg::Promise<T> <-> JS Promise
Expand Down
18 changes: 7 additions & 11 deletions src/workerd/jsg/value.h
Expand Up @@ -843,20 +843,16 @@ class ArrayWrapper {
// (the kj::Array object holds a Global to the unwrapped ArrayBuffer)
// - ArrayBufferView -> kj::Array<kj::byte>
// (the kj::Array object holds a Global to the unwrapped ArrayBufferView's backing buffer)
// - ArrayBuffer -> kj::ArrayPtr<kj::byte>
// - ArrayBufferView -> kj::ArrayPtr<kj::byte>
//
// Note that there is no wrapping conversion from kj::ArrayPtr<kj::byte>, since it does not own its
// own buffer -- fine in C++, but problematic in a GC language like JS. Since the JS object would
// need to hold shared ownership of its buffer, but we don't know who owns the buffer viewed by an
// ArrayPtr, we would need to create a copy of the buffer. You should probably make that copy
// explicitly with a kj::heapArray() call, meaning we only need to wrap kj::ArrayPtr<kj::byte>s.
// Note that there are no conversions for kj::ArrayPtr<kj::byte>, since it does not own its own
// buffer -- fine in C++, but problematic in a GC language like JS. Restricting the interface to
// only operate on owned arrays makes memory management simpler and safer in both directions.
//
// Logically a kj::Array<byte> could be considered analogous to a Uint8Array in JS, and for a time
// that was the wrapping conversion implemented by this wrapper. However, the most common use cases
// in web platform APIs involve accepting BufferSources for processing as immutable input and
// returning ArrayBuffers. Since a kj::byte does not map to any JavaScript primitive, establishing
// a mapping between ArrayBuffer/ArrayBufferView and Array<byte>/ArrayPtr<byte> is unambiguous and
// a mapping between ArrayBuffer/ArrayBufferView and Array<byte> is unambiguous and
// convenient. The few places where a specific TypedArray is expected (e.g. Uint8Array) can be
// handled explicitly with a v8::Local<v8::Uint8Array> (or other appropriate TypedArray type).
//
Expand All @@ -870,13 +866,13 @@ class ArrayWrapper {
// This suggests the following rules of thumb:
//
// 1. If a BufferSource parameter is used as input to a:
// - synchronous method: accept a `kj::ArrayPtr<const kj::byte>`.
// - synchronous method: accept a `kj::Array<const kj::byte>`.
// - asynchronous method (user is allowed to re-use the buffer during processing): accept a
// `kj::ArrayPtr<const kj::byte>` and explicitly copy its bytes.
// `kj::Array<const kj::byte>` and explicitly copy its bytes.
//
// 2. If a method accepts an ArrayBufferView that it is expected to mutate:
// - accept a `v8::Local<v8::ArrayBufferView>` explicitly (handled by V8HandleWrapper in
// type-wrapper.h) rather than a `kj::ArrayPtr<kj::byte>` -- otherwise your method's contract
// type-wrapper.h) rather than a `kj::Array<kj::byte>` -- otherwise your method's contract
// will be wider than intended.
// - use `jsg::asBytes()` as a quick way to get a `kj::ArrayPtr<kj::byte>` view onto it.
//
Expand Down