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

util: add isArrayBufferDetached method #45512

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions doc/api/util.md
Original file line number Diff line number Diff line change
Expand Up @@ -2024,6 +2024,30 @@ util.types.isAnyArrayBuffer(new ArrayBuffer()); // Returns true
util.types.isAnyArrayBuffer(new SharedArrayBuffer()); // Returns true
```
anonrig marked this conversation as resolved.
Show resolved Hide resolved

### `util.types.isArrayBufferDetached(value)`

<!-- YAML
added: REPLACEME
-->

* `value` {any}
* Returns: {boolean}

Returns `true` if the value is a built-in [`ArrayBuffer`][] and
is detached. For example, we can end up with a detached buffer
when using a BYOB (bring your own buffer) on a ReadableStream.
Detached arrays have a `byteLength` of 0, and their contents can
not be accessed in JavaScript.

```js
util.types.isArrayBufferDetached(null); // Returns false
util.types.isArrayBufferDetached(new ArrayBuffer()); // Returns false
anonrig marked this conversation as resolved.
Show resolved Hide resolved

const { buffer } = new Uint8Array([1, 2, 3]);
new MessageChannel().port1.postMessage('', [buffer]);
util.types.isArrayBufferDetached(buffer); // Returns true
```

### `util.types.isArrayBufferView(value)`

<!-- YAML
Expand Down
14 changes: 13 additions & 1 deletion lib/internal/util/types.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,15 @@
'use strict';

const {
ArrayBuffer,
ArrayBufferIsView,
ArrayBufferPrototypeGetByteLength,
ObjectDefineProperties,
TypedArrayPrototypeGetSymbolToStringTag,
} = primordials;

const { isArrayBufferDetached: _isArrayBufferDetached, ...internalTypes } = internalBinding('types');

function isTypedArray(value) {
return TypedArrayPrototypeGetSymbolToStringTag(value) !== undefined;
}
Expand Down Expand Up @@ -54,8 +58,16 @@ function isBigUint64Array(value) {
return TypedArrayPrototypeGetSymbolToStringTag(value) === 'BigUint64Array';
}

function isArrayBufferDetached(value) {
if (value instanceof ArrayBuffer && ArrayBufferPrototypeGetByteLength(value) === 0) {
Copy link
Contributor

@aduh95 aduh95 Nov 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't be surprised if that instanceof check had a dramatic impact on performance.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Referencing: https://github.com/RafaelGSS/nodejs-bench-operations/blob/main/RESULTS-v19.md#comparison-using-instanceof

I can change it to value != null && ... , and expect .byteLength to be the differentiator.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the best course of action is to make an internal isArrayBufferDetached without any instanceof check, and if we have a public one, it would return value instanceof ArrayBuffer && internal.isArrayBufferDetached(value).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, instanceof is brittle and can be forged, and doesn’t work cross-realm. In JS the way to check this is using the byteLength getter in a try/catch.

return _isArrayBufferDetached(value);
}
return false;
}

module.exports = {
...internalBinding('types'),
...internalTypes,
isArrayBufferDetached,
isArrayBufferView: ArrayBufferIsView,
isTypedArray,
isUint8Array,
Expand Down
6 changes: 3 additions & 3 deletions lib/internal/webstreams/readablestream.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ const {
} = internalBinding('messaging');

const {
isArrayBufferDetached,
isArrayBufferView,
isDataView,
} = require('internal/util/types');
Expand Down Expand Up @@ -104,7 +105,6 @@ const {
extractHighWaterMark,
extractSizeAlgorithm,
lazyTransfer,
isDetachedBuffer,
isViewedArrayBufferDetached,
isBrandCheck,
resetQueue,
Expand Down Expand Up @@ -669,7 +669,7 @@ class ReadableStreamBYOBRequest {
const viewBuffer = ArrayBufferViewGetBuffer(view);
const viewBufferByteLength = ArrayBufferGetByteLength(viewBuffer);

if (isDetachedBuffer(viewBuffer)) {
if (isArrayBufferDetached(viewBuffer)) {
throw new ERR_INVALID_STATE.TypeError('Viewed ArrayBuffer is detached');
}

Expand Down Expand Up @@ -2643,7 +2643,7 @@ function readableByteStreamControllerEnqueue(controller, chunk) {
if (pendingPullIntos.length) {
const firstPendingPullInto = pendingPullIntos[0];

if (isDetachedBuffer(firstPendingPullInto.buffer)) {
if (isArrayBufferDetached(firstPendingPullInto.buffer)) {
throw new ERR_INVALID_STATE.TypeError(
'Destination ArrayBuffer is detached',
);
Expand Down
17 changes: 2 additions & 15 deletions lib/internal/webstreams/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ const {
} = internalBinding('buffer');

const {
isArrayBufferDetached,
isPromise,
} = require('internal/util/types');

Expand Down Expand Up @@ -139,23 +140,10 @@ function transferArrayBuffer(buffer) {
return res;
}

function isDetachedBuffer(buffer) {
if (ArrayBufferGetByteLength(buffer) === 0) {
anonrig marked this conversation as resolved.
Show resolved Hide resolved
// TODO(daeyeon): Consider using C++ builtin to improve performance.
try {
new Uint8Array(buffer);
} catch (error) {
assert(error.name === 'TypeError');
return true;
}
}
return false;
}

function isViewedArrayBufferDetached(view) {
return (
ArrayBufferViewGetByteLength(view) === 0 &&
isDetachedBuffer(ArrayBufferViewGetBuffer(view))
isArrayBufferDetached(ArrayBufferViewGetBuffer(view))
);
}

Expand Down Expand Up @@ -256,7 +244,6 @@ module.exports = {
extractSizeAlgorithm,
lazyTransfer,
isBrandCheck,
isDetachedBuffer,
isPromisePending,
isViewedArrayBufferDetached,
peekQueueValue,
Expand Down
13 changes: 13 additions & 0 deletions src/node_types.cc
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,16 @@ static void IsBoxedPrimitive(const FunctionCallbackInfo<Value>& args) {
args[0]->IsSymbolObject());
}

static void IsArrayBufferDetached(const FunctionCallbackInfo<Value>& args) {
if (args[0]->IsArrayBuffer()) {
auto buffer = args[0].As<v8::ArrayBuffer>();
args.GetReturnValue().Set(buffer->WasDetached());
return;
}

args.GetReturnValue().Set(false);
}

void InitializeTypes(Local<Object> target,
Local<Value> unused,
Local<Context> context,
Expand All @@ -71,6 +81,8 @@ void InitializeTypes(Local<Object> target,

SetMethodNoSideEffect(context, target, "isAnyArrayBuffer", IsAnyArrayBuffer);
SetMethodNoSideEffect(context, target, "isBoxedPrimitive", IsBoxedPrimitive);
SetMethodNoSideEffect(
context, target, "isArrayBufferDetached", IsArrayBufferDetached);
}

} // anonymous namespace
Expand All @@ -82,6 +94,7 @@ void RegisterTypesExternalReferences(ExternalReferenceRegistry* registry) {

registry->Register(IsAnyArrayBuffer);
registry->Register(IsBoxedPrimitive);
registry->Register(IsArrayBufferDetached);
}
} // namespace node

Expand Down
11 changes: 10 additions & 1 deletion test/parallel/test-util-types.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ for (const [ value, _method ] of [
for (const key of Object.keys(types)) {
if ((types.isArrayBufferView(value) ||
types.isAnyArrayBuffer(value)) && key.includes('Array') ||
key === 'isBoxedPrimitive') {
key === 'isBoxedPrimitive' || key === 'isArrayBufferDetached') {
continue;
}

Expand All @@ -67,6 +67,15 @@ for (const [ value, _method ] of [
}
}

// Check detached array buffers.
{
[null, undefined].forEach((entry) => assert(types.isArrayBufferDetached(entry) === false));

const { buffer } = new Uint8Array([1, 2, 3]);
new MessageChannel().port1.postMessage('', [buffer]);
assert(types.isArrayBufferDetached(buffer));
}

// Check boxed primitives.
[
new Boolean(),
Expand Down
1 change: 1 addition & 0 deletions typings/internalBinding/types.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ declare function InternalBinding(binding: 'types'): {
isArrayBuffer(value: unknown): value is ArrayBuffer;
isArgumentsObject(value: unknown): value is ArrayLike<unknown>;
isBoxedPrimitive(value: unknown): value is (BigInt | Boolean | Number | String | Symbol);
isArrayBufferDetached(value: unknown): boolean;
isDataView(value: unknown): value is DataView;
isExternal(value: unknown): value is Object;
isMap(value: unknown): value is Map<unknown, unknown>;
Expand Down