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

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

Closed
penalosa opened this issue Jul 5, 2023 · 14 comments
Labels
promises Issues and PRs related to ECMAScript promises. stream Issues and PRs related to the stream subsystem.

Comments

@penalosa
Copy link

penalosa commented Jul 5, 2023

Version

v20.4.0

Platform

Darwin 22.1.0 Darwin Kernel Version 22.1.0: Sun Oct 9 20:15:09 PDT 2022; root:xnu-8792.41.9~2/RELEASE_ARM64_T6000 arm64 arm Darwin

Subsystem

No response

What steps will reproduce the bug?

Run:

import { Blob } from "node:buffer";

const stream = new Blob(["Hello world"]).stream();

await Promise.resolve();

await stream.pipeTo(new WritableStream());
console.log("Done");

with node. The process will exit with code 13

How often does it reproduce? Is there a required condition?

Always reproduces when the line await Promise.resolve(); is included (or awaiting any promise). If that line is commented out the program prints "Done"

What is the expected behavior? Why is that the expected behavior?

I expect the sample program to run to completion and print "Done", as it does in node v20.3.1

What do you see instead?

Node exiting with code 13:

> node sample.mjs

> echo $?
13

Additional information

No response

@bnoordhuis
Copy link
Member

It's mentioned in the documentation on node's exit codes:

  • 13 Unfinished Top-Level Await: await was used outside of a function
    in the top-level code, but the passed Promise never resolved.

@bnoordhuis bnoordhuis closed this as not planned Won't fix, can't repro, duplicate, stale Jul 6, 2023
@bnoordhuis bnoordhuis added the invalid Issues and PRs that are invalid. label Jul 6, 2023
@bnoordhuis
Copy link
Member

bnoordhuis commented Jul 6, 2023

I expect the sample program to run to completion and print "Done", as it does in node v20.3.1

Sorry, I should have mentioned the old behavior was probably a bug. The new behavior is the expected behavior.

@bnoordhuis
Copy link
Member

On second thought, I think this is indeed a bug, or at least an unintentional change in behavior introduced in f9c0d5a, cc @debadree25.

@bnoordhuis bnoordhuis reopened this Jul 6, 2023
@bnoordhuis bnoordhuis added stream Issues and PRs related to the stream subsystem. promises Issues and PRs related to ECMAScript promises. and removed invalid Issues and PRs that are invalid. labels Jul 6, 2023
@debadree25
Copy link
Member

hello!

hmm this does look like a bug so in my PR fixing hanging promises thing i am enqueing data during the microtask phase maybe thats causing some weird mixup in the event loop, I am investigating this! in the meanwhile do you think i should make a revert? in case this is breaking things?

@bricss
Copy link

bricss commented Jul 6, 2023

I have an issue with blobs as well that started with Node v20.4.0, but it's related to blob to stream conversion and http2 client transfer, which is leading to readable stream to never close, hanging connection forever ♾️
I will file a separate issue with reproduction example a bit later ⌚
But atm it's reproducible by simply running tests for this npm package 📦 https://www.npmjs.com/package/rekwest

@bricss
Copy link

bricss commented Jul 6, 2023

Here we go -> #48685

@asamuzaK
Copy link

Similar bug with fetch.

const file = new Blob(['<svg></svg>'], {
  type: 'image/svg+xml'
});
const url = URL.createObjectURL(file);
const res = await fetch(url);
const blob = await res.blob(); // does not resolve
console.log(blob.size); // nothing logged

It was working fine until v20.3.1.

There is also #48157 for blob related fixes.

@bricss
Copy link

bricss commented Jul 22, 2023

ISTM, that #48232 fixed hanging promises, but causing hanging streams right now 🤔
And it might be missing pending.resolve() inside of queueMicrotask() 🔬

@debadree25
Copy link
Member

And it might be missing pending.resolve() inside of queueMicrotask() 🔬

But it does call promise resolve and reject inside the readNext fn

Nonetheless sorry for the delay I shall get to investigating it by tomorrow!

@bricss
Copy link

bricss commented Jul 22, 2023

It's highly likely that manual backpressure check results in a deadlock after stream drain 🙄

@debadree25
Copy link
Member

Okay adding the promise resolve indeed fixes things i am opening a PR, in the process of adding tests

@debadree25
Copy link
Member

Okay on testing, it breaks existing tests 🥲 guess would be best to revert it for now i think

@bricss
Copy link

bricss commented Jul 26, 2023

Try to revert it and then add queueMicrotask() w/o manual backpressure check 💡

@debadree25
Copy link
Member

without the backpressure check memory leak we get memory leak, nonetheless let me try until to night else reverting the full PR

bellbind added a commit to bellbind/node that referenced this issue Jul 26, 2023
bellbind added a commit to bellbind/node that referenced this issue Jul 26, 2023
…8916

Add lacked calling resolve() for finish ReadableStream source.pull().

Fixes: nodejs#48668
Fixes: nodejs#48916
Fixes: nodejs#48232
Refs: 8cc1438
bellbind added a commit to bellbind/node that referenced this issue Jul 27, 2023
Add lacked calling resolve() for finish ReadableStream source.pull().

Fixes: nodejs#48668
Fixes: nodejs#48916
Fixes: nodejs#48232
Refs: 8cc1438
bellbind added a commit to bellbind/node that referenced this issue Jul 27, 2023
Add lacked calling resolve() for finish ReadableStream source.pull().

Fixes: nodejs#48668
Fixes: nodejs#48916
Fixes: nodejs#48232
Refs: 8cc1438
bellbind added a commit to bellbind/node that referenced this issue Jul 27, 2023
Add lacked calling resolve() for finish ReadableStream source.pull().

Fixes: nodejs#48668
Fixes: nodejs#48916
Fixes: nodejs#48232
Refs: 8cc1438
bellbind added a commit to bellbind/node that referenced this issue Jul 27, 2023
Add lacked calling resolve() for finish ReadableStream source.pull().

Fixes: nodejs#48668
Fixes: nodejs#48916
Fixes: nodejs#48232
Refs: 8cc1438
bellbind added a commit to bellbind/node that referenced this issue Jul 27, 2023
Add lacked calling resolve() for finish ReadableStream source.pull().

Fixes: nodejs#48668
Fixes: nodejs#48916
Fixes: nodejs#48232
Refs: 8cc1438
bellbind added a commit to bellbind/node that referenced this issue Jul 27, 2023
Add lacked calling resolve() for finish ReadableStream source.pull().

Fixes: nodejs#48668
Fixes: nodejs#48916
Fixes: nodejs#48232
Refs: nodejs@8cc1438
bellbind added a commit to bellbind/node that referenced this issue Jul 28, 2023
Add lacked calling resolve() for finish ReadableStream source.pull().

Fixes: nodejs#48668
Fixes: nodejs#48916
Fixes: nodejs#48232
Refs: nodejs@8cc1438
debadree25 pushed a commit to bellbind/node that referenced this issue Aug 12, 2023
Add lacked calling resolve() for finish ReadableStream source.pull().

Fixes: nodejs#48668
Fixes: nodejs#48916
Fixes: nodejs#48232
Refs: nodejs@8cc1438
Ceres6 pushed a commit to Ceres6/node that referenced this issue Aug 14, 2023
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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matthew Aitken <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this issue Aug 14, 2023
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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matthew Aitken <[email protected]>
RafaelGSS pushed a commit that referenced this issue Aug 15, 2023
Add lacked calling resolve() for finish ReadableStream source.pull().

Fixes: #48668
Fixes: #48916
Fixes: #48232
Refs: 8cc1438
PR-URL: #48935
Reviewed-By: Debadree Chatterjee <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matthew Aitken <[email protected]>
RafaelGSS pushed a commit to RafaelGSS/node that referenced this issue Aug 15, 2023
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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matthew Aitken <[email protected]>
rluvaton pushed a commit to rluvaton/node that referenced this issue Aug 15, 2023
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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matthew Aitken <[email protected]>
RafaelGSS pushed a commit that referenced this issue Aug 16, 2023
Add lacked calling resolve() for finish ReadableStream source.pull().

Fixes: #48668
Fixes: #48916
Fixes: #48232
Refs: 8cc1438
PR-URL: #48935
Reviewed-By: Debadree Chatterjee <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matthew Aitken <[email protected]>
RafaelGSS pushed a commit that referenced this issue Aug 17, 2023
Add lacked calling resolve() for finish ReadableStream source.pull().

Fixes: #48668
Fixes: #48916
Fixes: #48232
Refs: 8cc1438
PR-URL: #48935
Reviewed-By: Debadree Chatterjee <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matthew Aitken <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
promises Issues and PRs related to ECMAScript promises. stream Issues and PRs related to the stream subsystem.
Projects
None yet
5 participants