node icon indicating copy to clipboard operation
node copied to clipboard

`.pipeTo(Writable.toWeb(process.stdout))` returns a never-settling Promise

Open mikaelkaron opened this issue 1 year ago • 2 comments

Version

  • v22.9.0
  • v22.12.0

Platform

Linux a54ff73afbfe 5.15.153.1-microsoft-standard-WSL2 #1 SMP Fri Mar 29 23:14:13 UTC 2024 x86_64 GNU/Linux
Linux SURFACE9PRO 5.15.153.1-microsoft-standard-WSL2 #1 SMP Fri Mar 29 23:14:13 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

node:stream

What steps will reproduce the bug?

create file test.mjs

import { Readable, Writable } from 'node:stream'

await Readable.toWeb(process.stdin).pipeTo(Writable.toWeb(process.stdout))

run

echo test | node test.mjs 
test
Warning: Detected unsettled top-level await at file:///workspace/test.mjs:2
await Readable.toWeb(process.stdin).pipeTo(Writable.toWeb(process.stdout))
^

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

always

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

no warning, await to work as expected

What do you see instead?

warning about unsettled top-level await is shown

Additional information

No response

mikaelkaron avatar Dec 05 '24 01:12 mikaelkaron

Some (small followups):

Simplified to get rid of Readable.toWeb

import { Writable } from 'node:stream'
import { ReadableStream } from 'node:stream/web'

await ReadableStream.from(["test"]).pipeTo(Writable.toWeb(process.stdout))
node test.mjs 
testWarning: Detected unsettled top-level await at file:///workspace/test.mjs:4
await ReadableStream.from(["test"]).pipeTo(Writable.toWeb(process.stdout))
^

Without Writable.toWeb there's no error:

import { ReadableStream } from 'node:stream/web'

await ReadableStream.from(["test"]).pipeTo(new WritableStream())

And just to make sure let's implement a naive implementation of a WritableStream

import { ReadableStream } from 'node:stream/web'
import { setTimeout } from 'node:timers/promises'

const streamWritable = process.stdout

await ReadableStream.from(["test"]).pipeTo(new WritableStream({
  write(chunk) {
    if (streamWritable.writableNeedDrain || !streamWritable.write(chunk)) {
      return new Promise(resolve => streamWritable.once('drain', resolve))
    }
  },
  close() {
    return setTimeout(1000)
  }
}))

mikaelkaron avatar Dec 05 '24 16:12 mikaelkaron

The problem is not that there's a warning, it's that the promise never resolves:

$ node -e '
const {Writable} = require("node:stream");
const {ReadableStream} = require("node:stream/web");
ReadableStream.from(["test\n"]).pipeTo(Writable.toWeb(process.stdout)).then(() => console.log("Done"), console.error)'
test
$ echo $?
0

As you can see, the console.log("Done") is never reached. The warning is a mere consequence of awaiting a never-settling promise on the top-level scope.

aduh95 avatar Dec 05 '24 20:12 aduh95

The problem is not that there's a warning, it's that the promise never resolves:

Indeed. I had a few extra minutes to do some initial testing. I looks like when the code gets here closed is undefined which returns a promise here.

I'm guessing closed is supposed to be handled in cleanup but when I debug I never get there which leads me to suspect that eos is not running the supplied callback for some reason.

So that was me not setting up my debugging env propperly, the callback is executed, so I'll have to dig some more.

mikaelkaron avatar Dec 07 '24 12:12 mikaelkaron

I decided to go in a slightly different direction and started testing with different node version, and it look like this works in v21.7.3, but is broken in v22.0.0. Also tested in in v23.3.0 where it's also broken.

mikaelkaron avatar Dec 08 '24 19:12 mikaelkaron

It could be related to a V8 update, or another semver-major commit of v22.x but I don't see any related

aduh95 avatar Dec 09 '24 11:12 aduh95

The reason you won't see the warning in v21.7.3 but see in v22.0.0 is because since v22.0.0 it starts to detect unsettled top-level await - see this commit - more detail please take a look at this PR https://github.com/nodejs/node/pull/51999.

I have tested v21.7.3 locally and the issue described by @aduh95 (here) still exists.

jakecastelli avatar Dec 09 '24 11:12 jakecastelli

Oh well, I guess it's nice to have found something new rather than a regression 🤷

mikaelkaron avatar Dec 09 '24 12:12 mikaelkaron

The issue persists in node 23.11.0

029A-h avatar Apr 17 '25 12:04 029A-h

I found a workaround which prevents throwing Warning: Detected unsettled top-level await.

await ReadableStream.from(['test']).pipeTo(Writable.toWeb(process.stdout), { preventClose: true })

029A-h avatar Apr 17 '25 12:04 029A-h