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 daeyeon.dev@gmail.com

This reverts commit 6f6ba3e. Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

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

+function isDetachedBuffer(buffer) {
+ if (!isArrayBuffer(buffer))
+ throw new ERR_INVALID_ARG_TYPE('buffer', 'ArrayBuffer', buffer);


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 daeyeon.dev@gmail.com

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

+ if (!isArrayBufferView(view)) {
+ throw new ERR_INVALID_ARG_TYPE(
+ 'view',
+ ['Buffer', 'TypedArray', 'DataView'],
+ view,
+ );
+ }


Can you use validateBuffer instead?
node/lib/internal/validators.js
Lines 200 to 206 in d83446b
constvalidateBuffer=hideStackFrames((buffer,name='buffer')=>{
if(!isArrayBufferView(buffer)){
thrownewERR_INVALID_ARG_TYPE(name,
['Buffer','TypedArray','DataView'],
buffer);
}
});

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


-function isArrayBufferDetached(buffer) {
+function isDetachedBuffer(buffer) {
+ assert(isArrayBuffer(buffer));


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

 }

 function isViewedArrayBufferDetached(view) {
+ assert(isArrayBufferView(view));


same here
Suggested change
assert(isArrayBufferView(view));

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

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


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

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


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 daeyeon.dev@gmail.com

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 daeyeon.dev@gmail.com PR-URL: #44114 Refs: #43866 Reviewed-By: LiviaMedeiros <livia@cirno.name> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

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

Depends on #43866

stream: fix isDetachedBuffer validations
e5e32c0
Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

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

Comment threadlib/internal/webstreams/readablestream.js Outdated
Comment threadlib/internal/webstreams/readablestream.js

daeyeon added 2 commits August 4, 2022 17:19

fixup: check zero and add asserts
50c7728
Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

fixup: use isDetachedBuffer only
6f6ba3e
Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

LiviaMedeiros reviewed Aug 4, 2022
View reviewed changes

Comment threadlib/internal/webstreams/util.js
Comment threadlib/internal/webstreams/readablestream.js Outdated
Comment threadlib/internal/webstreams/readablestream.js

daeyeon added 3 commits August 4, 2022 20:39

revert "fixup: use isDetachedBuffer only"
782c71a
This reverts commit 6f6ba3e. Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

fixup: remove redundant isDetachedBuffer
e44effa
Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

fixup: improve the checker
6fd3ea1
Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

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 daeyeon.dev@gmail.com

fixup: use assert for isDetachedBuffer
fe816cd
Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

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-actionsBot 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 daeyeon.dev@gmail.com

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

github-actionsBot 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:mainAug 18, 2022

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

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

	constvalidateBuffer=hideStackFrames((buffer,name='buffer')=>{
	if(!isArrayBufferView(buffer)){
	thrownewERR_INVALID_ARG_TYPE(name,
	['Buffer','TypedArray','DataView'],
	buffer);
	}
	});

Uh oh!

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

juanarbol commented Sep 30, 2022•
edited
Loading