stream: fix `isDetachedBuffer` validations in `ReadableStream` by daeyeon · Pull Request #44114 · nodejs/node

daeyeon · 2022-08-03T12:11:22Z

This fixes validations below related to isDetachedBuffer using a function introduced in #43866.

https://streams.spec.whatwg.org/#rs-byob-request-respond

The respond(bytesWritten) method steps are:

If this.[[controller]] is undefined, throw a TypeError exception.
If ! IsDetachedBuffer(this.[[view]].[[ArrayBuffer]]) is true, throw a TypeError exception.

https://streams.spec.whatwg.org/#byob-reader-read

The read(view) method steps are:

If view.[[ByteLength]] is 0, return a promise rejected with a TypeError exception.
If view.[[ViewedArrayBuffer]].[[ArrayBufferByteLength]] is 0, return a promise rejected with a TypeError exception.
If ! IsDetachedBuffer(view.[[ViewedArrayBuffer]]) is true, return a promise rejected with a TypeError exception.

https://streams.spec.whatwg.org/#readable-byte-stream-controller-enqueue

If controller.[[pendingPullIntos]] is not empty,

8.1 Let firstPendingPullInto be controller.[[pendingPullIntos]][0].
8.2 If ! IsDetachedBuffer(firstPendingPullInto’s buffer) is true, throw a TypeError exception.

Refs: #43866 (review)

Signed-off-by: Daeyeon Jeong [email protected]

lib/internal/webstreams/readablestream.js

Signed-off-by: Daeyeon Jeong [email protected]

lib/internal/webstreams/util.js

lib/internal/webstreams/readablestream.js

This reverts commit 6f6ba3e. Signed-off-by: Daeyeon Jeong [email protected]

Signed-off-by: Daeyeon Jeong [email protected]

lib/internal/webstreams/readablestream.js

LiviaMedeiros · 2022-08-07T03:57:46Z

lib/internal/webstreams/util.js

Non-blocking comment: this change is fine in context of this PR, but it probably should be rewritten in a follow-up.

It would be better if is* functions always returned boolean without throwing. If we want to throw, validate* function is preferable.
Since either true or false won't make sense if provided value is not a buffer, it looks like an acceptable temporal solution.

Indeed, it looks better to follow the coding convention.

I updated the util using assert instead, since, as you mentioned, either true or false won't make sense if a provided value is not a buffer. We can guarantee a passing value is a buffer since it's internal util. PTAL.

Signed-off-by: Daeyeon Jeong [email protected]

nodejs-github-bot · 2022-08-08T13:02:58Z

CI: https://ci.nodejs.org/job/node-test-pull-request/45921/

aduh95 · 2022-08-17T18:11:54Z

lib/internal/webstreams/readablestream.js

Can you use validateBuffer instead?

node/lib/internal/validators.js

Lines 200 to 206 in d83446b

const validateBuffer = hideStackFrames((buffer, name = 'buffer') => {

if (!isArrayBufferView(buffer)) {

throw new ERR_INVALID_ARG_TYPE(name,

['Buffer', 'TypedArray', 'DataView'],

buffer);

}

});

aduh95 · 2022-08-17T18:15:36Z

lib/internal/webstreams/util.js

Is this useful? I think the error thrown by V8 enough (and clearer).

Suggested change

assert(isArrayBuffer(buffer));

On second thought, seemingly it can be deleted in the current version of this PR. Fixed.

aduh95 · 2022-08-17T18:15:55Z

lib/internal/webstreams/util.js

same here

Suggested change

assert(isArrayBufferView(view));

aduh95 · 2022-08-17T18:17:50Z

test/parallel/test-whatwg-readablebytestream-bad-buffers-and-views.js

Suggested change

reader

.read(view)

.then(common.mustNotCall())

.catch(

common.mustCall(

common.expectsError({

code: 'ERR_INVALID_STATE',

name: 'TypeError',

}),

),

);

assert.rejects(

reader.read(view),

{

code: 'ERR_INVALID_STATE',

name: 'TypeError',

}

).then(common.mustCall());

aduh95 · 2022-08-17T18:19:02Z

test/parallel/test-whatwg-readablebytestream-bad-buffers-and-views.js

Suggested change

reader

.read(view)

.then(common.mustNotCall())

.catch(

common.mustCall(

common.expectsError({

code: 'ERR_INVALID_STATE',

name: 'TypeError',

}),

),

);

assert.rejects(

reader.read(view),

{

code: 'ERR_INVALID_STATE',

name: 'TypeError',

}

).then(common.mustCall());

Signed-off-by: Daeyeon Jeong [email protected]

daeyeon · 2022-08-18T00:52:12Z

@aduh95 Applied the suggestions, PTAL.

nodejs-github-bot · 2022-08-18T02:26:48Z

CI: https://ci.nodejs.org/job/node-test-pull-request/46057/

nodejs-github-bot · 2022-08-18T08:24:36Z

Landed in 937520a

Signed-off-by: Daeyeon Jeong [email protected] PR-URL: #44114 Refs: #43866 Reviewed-By: LiviaMedeiros <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

Signed-off-by: Daeyeon Jeong [email protected] PR-URL: nodejs#44114 Refs: nodejs#43866 Reviewed-By: LiviaMedeiros <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

juanarbol · 2022-09-30T03:55:09Z

Depends on #43866

stream: fix isDetachedBuffer validations

e5e32c0

Signed-off-by: Daeyeon Jeong [email protected]

nodejs-github-bot added the needs-ci PRs that need a full CI run. label Aug 3, 2022

LiviaMedeiros reviewed Aug 4, 2022

View reviewed changes

lib/internal/webstreams/readablestream.js Outdated Show resolved Hide resolved

lib/internal/webstreams/readablestream.js Show resolved Hide resolved

daeyeon added 2 commits August 4, 2022 17:19

fixup: check zero and add asserts

50c7728

Signed-off-by: Daeyeon Jeong [email protected]

fixup: use isDetachedBuffer only

6f6ba3e

Signed-off-by: Daeyeon Jeong [email protected]

LiviaMedeiros reviewed Aug 4, 2022

View reviewed changes

lib/internal/webstreams/util.js Show resolved Hide resolved

lib/internal/webstreams/readablestream.js Outdated Show resolved Hide resolved

lib/internal/webstreams/readablestream.js Show resolved Hide resolved

daeyeon added 3 commits August 4, 2022 20:39

revert "fixup: use isDetachedBuffer only"

782c71a

This reverts commit 6f6ba3e. Signed-off-by: Daeyeon Jeong [email protected]

fixup: remove redundant isDetachedBuffer

e44effa

Signed-off-by: Daeyeon Jeong [email protected]

fixup: improve the checker

6fd3ea1

Signed-off-by: Daeyeon Jeong [email protected]

daeyeon added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Aug 6, 2022

LiviaMedeiros reviewed Aug 7, 2022

View reviewed changes

LiviaMedeiros added the web streams label Aug 7, 2022

daeyeon added 2 commits August 7, 2022 17:42

fixup: change error message properly

0cc34f4

Signed-off-by: Daeyeon Jeong [email protected]

fixup: use assert for isDetachedBuffer

fe816cd

Signed-off-by: Daeyeon Jeong [email protected]

LiviaMedeiros approved these changes Aug 8, 2022

View reviewed changes

LiviaMedeiros added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 8, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 8, 2022

This comment was marked as outdated.

Sign in to view

This was referenced Aug 9, 2022

CI Reliability 2022-08-09 nodejs/reliability#341

Open

CI Reliability 2022-08-10 nodejs/reliability#342

Open

daeyeon added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 10, 2022

This was referenced Aug 11, 2022

CI Reliability 2022-08-11 nodejs/reliability#343

Open

CI Reliability 2022-08-12 nodejs/reliability#344

Open

aduh95 reviewed Aug 17, 2022

View reviewed changes

fixup: apply suggestions

58da0d8

Signed-off-by: Daeyeon Jeong [email protected]

daeyeon added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 18, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 18, 2022

aduh95 approved these changes Aug 18, 2022

View reviewed changes

aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 18, 2022

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 18, 2022

nodejs-github-bot merged commit 937520a into nodejs:main Aug 18, 2022

daeyeon deleted the main.use-same-validator-220728.Thu.7686 branch August 18, 2022 08:40

ruyadorno pushed a commit that referenced this pull request Aug 23, 2022

stream: fix isDetachedBuffer validations

a3ac445

Signed-off-by: Daeyeon Jeong [email protected] PR-URL: #44114 Refs: #43866 Reviewed-By: LiviaMedeiros <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

ruyadorno mentioned this pull request Aug 23, 2022

v18.8.0 release proposal #44353

Merged

juanarbol added the dont-land-on-v16.x label Sep 30, 2022

	const validateBuffer = hideStackFrames((buffer, name = 'buffer') => {
	if (!isArrayBufferView(buffer)) {
	throw new ERR_INVALID_ARG_TYPE(name,
	['Buffer', 'TypedArray', 'DataView'],
	buffer);
	}
	});

-  reader
-    .read(view)
-    .then(common.mustNotCall())
-    .catch(
-      common.mustCall(
-        common.expectsError({
-          code: 'ERR_INVALID_STATE',
-          name: 'TypeError',
-        }),
-      ),
-    );
+  assert.rejects(
+    reader.read(view),
+    {
+      code: 'ERR_INVALID_STATE',
+      name: 'TypeError',
+    }
+  ).then(common.mustCall());

Uh oh!

Conversation

daeyeon commented Aug 3, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

nodejs-github-bot commented Aug 8, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daeyeon commented Aug 18, 2022

Uh oh!

nodejs-github-bot commented Aug 18, 2022

Uh oh!

nodejs-github-bot commented Aug 18, 2022

Uh oh!

juanarbol commented Sep 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Comments

juanarbol commented Sep 30, 2022 •

edited

Loading