lib: fix blob.stream() causing hanging promises by debadree25 · Pull Request #48232 · nodejs/node

debadree25 · 2023-05-28T19:25:57Z

Running the following script on node v20.2.0

async function fn() {
  const b = new Blob([1, 2, 3, 4, 5, 5, 6, 6]).stream().getReader();

  const chunks = [];

  while (true) {
    const { done, value } = await b.read();
    chunks.push(value);
    console.log('read value', value);
    if (done) break;
  }

  console.log(chunks);
}

await fn();

would cause a promise hang and no output, In this PR have updated the pulling logic in blob.stream() to keep pulling until we reach the end, similar to how blob.arrayBuffer() is implemented.

Refs: #47993 (comment)

Refs: nodejs#47993 (comment)

debadree25 · 2023-05-28T19:37:16Z

cc @nodejs/whatwg-stream

nodejs-github-bot · 2023-05-29T19:08:53Z

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

mcollina

From my understanding of the code, this will read all the file without any backpressure kicking in. This is a recipe for memory leaks.

debadree25 · 2023-05-30T10:42:18Z

Ah good point, trying to add checks for the same

debadree25 · 2023-05-30T13:26:42Z

Hey @mcollina in the latest commit I have added some manual backpressure checking and a test too, does this look ok?

nodejs-github-bot · 2023-05-31T09:18:36Z

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

nodejs-github-bot · 2023-06-04T06:05:07Z

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

nodejs-github-bot · 2023-06-05T06:28:15Z

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

nodejs-github-bot · 2023-06-07T07:49:45Z

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

debadree25 · 2023-06-07T07:50:34Z

ping @mcollina

mcollina

Can you please add a test where the flow control mechanism kicks in? I would expect to see c.desiredSize turn negative.

lib/internal/blob.js

Co-authored-by: Matteo Collina <matteo.collina@gmail.com>

debadree25 · 2023-06-07T09:02:17Z

Can you please add a test where the flow control mechanism kicks in? I would expect to see c.desiredSize turn negative.

Sure adding tests for that

- also work example code in nodejs#48232

…8916 Add lacked calling resolve() for finish ReadableStream source.pull(). Fixes: nodejs#48668 Fixes: nodejs#48916 Fixes: nodejs#48232 Refs: 8cc1438

Add lacked calling resolve() for finish ReadableStream source.pull(). Fixes: nodejs#48668 Fixes: nodejs#48916 Fixes: nodejs#48232 Refs: 8cc1438

Add lacked calling resolve() for finish ReadableStream source.pull(). Fixes: nodejs#48668 Fixes: nodejs#48916 Fixes: nodejs#48232 Refs: nodejs@8cc1438

Add lacked calling resolve() for finish ReadableStream source.pull(). Fixes: #48668 Fixes: #48916 Fixes: #48232 Refs: 8cc1438 PR-URL: #48935 Reviewed-By: Debadree Chatterjee <debadree333@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>

Refs: nodejs#47993 (comment) PR-URL: nodejs#48232 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

Add lacked calling resolve() for finish ReadableStream source.pull(). Fixes: nodejs#48668 Fixes: nodejs#48916 Fixes: nodejs#48232 Refs: nodejs@8cc1438 PR-URL: nodejs#48935 Reviewed-By: Debadree Chatterjee <debadree333@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>

Refs: nodejs#47993 (comment) PR-URL: nodejs#48232 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

Add lacked calling resolve() for finish ReadableStream source.pull(). Fixes: nodejs#48668 Fixes: nodejs#48916 Fixes: nodejs#48232 Refs: nodejs@8cc1438 PR-URL: nodejs#48935 Reviewed-By: Debadree Chatterjee <debadree333@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>

Add lacked calling resolve() for finish ReadableStream source.pull(). Fixes: #48668 Fixes: #48916 Fixes: #48232 Refs: 8cc1438 PR-URL: #48935 Reviewed-By: Debadree Chatterjee <debadree333@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>

Add lacked calling resolve() for finish ReadableStream source.pull(). Fixes: nodejs#48668 Fixes: nodejs#48916 Fixes: nodejs#48232 Refs: nodejs@8cc1438 PR-URL: nodejs#48935 Reviewed-By: Debadree Chatterjee <debadree333@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>

Add lacked calling resolve() for finish ReadableStream source.pull(). Fixes: #48668 Fixes: #48916 Fixes: #48232 Refs: 8cc1438 PR-URL: #48935 Reviewed-By: Debadree Chatterjee <debadree333@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>

ruyadorno · 2023-09-07T02:38:25Z

This does not land cleanly in v18.x-staging and will need manual backport in case you want this to land on v18.

debadree25 · 2023-09-07T06:29:03Z

Hey @ruyadorno will open a backport pr for this also note that this PR did introduce a bug 😅 which was fixed by #48935 should i include both the commits in the backport PR?

lib: fix blob.stream() causing hanging promises

24ef7d6

Refs: nodejs#47993 (comment)

nodejs-github-bot added the needs-ci PRs that need a full CI run. label May 28, 2023

debadree25 added the web streams label May 28, 2023

debadree25 mentioned this pull request May 28, 2023

byob reader support for Blob.stream() #47993

Closed

jasnell approved these changes May 29, 2023

View reviewed changes

debadree25 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels May 29, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 29, 2023

mcollina requested changes May 30, 2023

View reviewed changes

This comment was marked as outdated.

Sign in to view

fixup! add back pressure

3f321a1

debadree25 requested a review from mcollina May 30, 2023 13:26

debadree25 removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 30, 2023

benjamingr approved these changes May 30, 2023

View reviewed changes

debadree25 added the request-ci Add this label to start a Jenkins CI on a PR. label May 31, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 31, 2023

github-actions bot mentioned this pull request Jun 5, 2023

CI Reliability 2023-06-05 nodejs/reliability#586

Open

30 tasks

github-actions bot mentioned this pull request Jun 6, 2023

CI Reliability 2023-06-06 nodejs/reliability#587

Open

20 tasks

mcollina requested changes Jun 7, 2023

View reviewed changes

lib/internal/blob.js Outdated Show resolved Hide resolved

Update lib/internal/blob.js

fd56c64

Co-authored-by: Matteo Collina <matteo.collina@gmail.com>

RafaelGSS mentioned this pull request Jul 3, 2023

v20.4.0 proposal #48643

Merged

bricss mentioned this pull request Jul 6, 2023

http2: connection hangs forever ♾️ when blob stream is piped 🚰 through client to server #48685

Closed

bricss mentioned this pull request Jul 22, 2023

Awaiting a Promise between creating a stream and consuming a stream causes Node to crash with code 13 #48668

Closed

bellbind added a commit to bellbind/node that referenced this pull request Jul 26, 2023

fix lib/internal/blob.js for nodejs#48668 nodejs#48916

e8dcf79

- also work example code in nodejs#48232

bellbind mentioned this pull request Jul 26, 2023

lib: fix lib/internal/blob.js Blob.stream() for #48668 #48916 #48930

Closed

bellbind mentioned this pull request Jul 27, 2023

lib: fix to add resolve() before return at Blob.stream()'s source.pull() #48935

Closed

bellbind mentioned this pull request Jul 27, 2023

lib: fix lacked resolve() in Blob.stream()'s source.pull() #48938

Closed

bellbind added a commit to bellbind/node that referenced this pull request Jul 27, 2023

ib: fix Blob.stream() for nodejs#48668 nodejs#48916

a3af3cb

Add lacked calling resolve() for finish ReadableStream source.pull(). Fixes: nodejs#48668 Fixes: nodejs#48916 Fixes: nodejs#48232 Refs: 8cc1438

ruyadorno added the backport-requested-v18.x label Sep 7, 2023

debadree25 deleted the fix/blob-stream-hang branch September 7, 2023 06:27

Uh oh!

Conversation

debadree25 commented May 28, 2023

Uh oh!

debadree25 commented May 28, 2023

Uh oh!

nodejs-github-bot commented May 29, 2023

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

debadree25 commented May 30, 2023

Uh oh!

This comment was marked as outdated.

debadree25 commented May 30, 2023

Uh oh!

nodejs-github-bot commented May 31, 2023

Uh oh!

nodejs-github-bot commented Jun 4, 2023

Uh oh!

nodejs-github-bot commented Jun 5, 2023

Uh oh!

nodejs-github-bot commented Jun 7, 2023

Uh oh!

debadree25 commented Jun 7, 2023

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

debadree25 commented Jun 7, 2023

Uh oh!

ruyadorno commented Sep 7, 2023

Uh oh!

debadree25 commented Sep 7, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants