Regression in Node.js v21.6.0

[mcollina]

#51255 makes the following test fail:

import { test } from 'node:test'
import assert from 'node:assert'

test('repro', async (t) => {
  const buf = new Uint8Array(1)
  const readable = new ReadableStream({
    start (controller) {
      controller.enqueue(buf)
      controller.close()
    }
  })

  const [out1, out2] = readable.tee()
  const cloned = structuredClone(out2, { transfer: [out2] })

  for await (const chunk of cloned) {
    assert.deepStrictEqual(chunk, buf)
  }

  for await (const chunk of out2) {
    assert.deepStrictEqual(chunk, buf)
  }

  for await (const chunk of out1) {
    assert.deepStrictEqual(chunk, buf)
  }
})

with:

✖ repro (5.626125ms)
  'Promise resolution is still pending but the event loop has already resolved'

ℹ tests 1
ℹ suites 0
ℹ pass 0
ℹ fail 0
ℹ cancelled 1
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 8.293458

✖ failing tests:

test at file:/Users/matteo/Repositories/node/repro.mjs:6:1
✖ repro (5.626125ms)
  'Promise resolution is still pending but the event loop has already resolved'

It seems that #51255 unveiled a problem of a lingering promise not being resolved properly after the two MessagePorts have been closed.

This emerged in undici, see nodejs/undici#2617 for more details.

[mcollina]

cc @jasnell

[H4ad]

Do you think the #51131 can fix both issues? Or did we just find a new bug after fixing the old bug?

[mcollina]

I think this unveiled a new bug. I'm not sure if #51131 fixes it, I would not expect it to, but I haven't time to track down that lingering promise.

[marco-ippolito]

I tested it on main but cannot reproduce it

if [ ! -r node ] || [ ! -L node ]; then ln -fs out/Release/node node; fi
14:46:43 ~/Documents/projects/forks/node main $ ./node test/parallel/test-regression.js
✔ repro (7.64375ms)
ℹ tests 1
ℹ suites 0
ℹ pass 1
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 10.764625

[mcollina]

This reproduces solidly on my end and undici CI on both Linux and Mac OS X, have you pulled the latest commits @marco-ippolito?

[marco-ippolito]

Yes, head at e133e5115a829aa7a445cfd8754df3a3dd586b0a cleaned up build too

[mcollina]

fun! :D

[RafaelGSS]

@marco-ippolito are you running it as a .mjs?

[marco-ippolito]

@marco-ippolito are you running it as a .mjs?

Right I was running it as cjs 😮 so its mjs only infact as mjs I can reproduce

[mcollina]

Even more fun!

[H4ad]

Using .mjs and #51131:

✔ repro (8.221ms)
ℹ tests 1
ℹ suites 0
ℹ pass 1
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 11.25954

I rebase main before running the test.

Edit: I needed to revert #51255 in order to make the test work.

[mcollina]

Based on nodejs/undici#2617 (comment), I propose we revert #51255 asap.

[Trott]

Am I mistaken or should this be closed now that #51491 has landed and 21.6.1 has been released?

[mcollina]

yes

[anonrig]

Is it intended to throw when you try to transfer an already detached array buffer?

var ab = new ArrayBuffer(1); 
structuredClone(ab, { transfer: [ab] }); 
structuredClone(ab, { transfer: [ab] });

[mcollina]

@anonrig you might want to open a fresh issue