Avoid structuredClone in Response() #2617

mcollina · 2024-01-16T11:41:53Z

Fixes #2615
Fixes #2618 (to make the tests pass on Node 20)

Note that this is caused by nodejs/node#51255 by @jasnell.
It can also be reproduced with:

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)
  }
})

which will fail 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'

Signed-off-by: Matteo Collina <[email protected]>

codecov-commenter · 2024-01-16T11:46:45Z

Codecov Report

Attention: 198 lines in your changes are missing coverage. Please review.

Comparison is base (e39a632) 85.54% compared to head (54c7c7d) 84.82%.
Report is 220 commits behind head on main.

Files	Patch %	Lines
lib/fetch/index.js	67.29%	52 Missing ⚠️
lib/fetch/util.js	37.17%	49 Missing ⚠️
lib/cache/cache.js	2.85%	34 Missing ⚠️
lib/handler/RetryHandler.js	74.35%	30 Missing ⚠️
lib/core/diagnostics.js	84.74%	9 Missing ⚠️
lib/api/readable.js	88.88%	5 Missing ⚠️
lib/fetch/headers.js	90.19%	5 Missing ⚠️
lib/client.js	93.65%	4 Missing ⚠️
lib/compat/dispatcher-weakref.js	57.14%	3 Missing ⚠️
lib/core/util.js	95.65%	2 Missing ⚠️
... and 4 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2617      +/-   ##
==========================================
- Coverage   85.54%   84.82%   -0.73%     
==========================================
  Files          76       81       +5     
  Lines        6858     7275     +417     
==========================================
+ Hits         5867     6171     +304     
- Misses        991     1104     +113

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Matteo Collina <[email protected]>

KhafraDev

This will cause the process to hang on v18 and v20

mcollina · 2024-01-16T15:01:37Z

This will cause the process to hang on v18 and v20

How? The tests are passing.

mcollina · 2024-01-16T15:27:33Z

After a bit of digging, structuredClone was introduced in 4b30d47, seemingly to pass a WPT test.

This was later fixed in c7eb5f0 by adding an additional .tee() as the test were not finishing due to nodejs/node#44985.

However, the given WPT test is marked as fail.

It does not seem we are losing anything by removing structuredClone, which is the source of the problem.

KhafraDev · 2024-01-16T15:38:21Z

Ah right, I was mistaking it for something else related to this code. When the WPT runner fails a test, it doesn't cause the process to exst with an error (this was changed so it could run with node to report fetch test stats). While those 2 tests are marked as failing, a bunch of others aren't, and those are failing:

 Started /home/runner/work/undici/undici/test/wpt/tests/fetch/api/response/response-clone.any.js
{
  status: 1,
  name: 'Check response clone use structureClone for teed ReadableStreams (Int8Arraychunk)',
  message: 'assert_not_equals: Buffer of cloned response stream is a clone of the original buffer got disallowed value object "0,0,0,0,0,0,0,0,0,0,0,0,0,0,0"',
  stack: 'Error\n' +
    '    at get_stack (evalmachine.<anonymous>:4551:21)\n' +
    '    at new AssertionError (evalmachine.<anonymous>:4544:22)\n' +
    '    at assert (evalmachine.<anonymous>:4528:19)\n' +
    '    at assert_not_equals (evalmachine.<anonymous>:1542:9)\n' +
    '    at assert_wrapper (evalmachine.<anonymous>:1445:30)\n' +
    '    at evalmachine.<anonymous>:122:13\n' +
    '    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)'
}
{
  status: 1,
  name: 'Check response clone use structureClone for teed ReadableStreams (Int16Arraychunk)',
  message: 'assert_not_equals: Buffer of cloned response stream is a clone of the original buffer got disallowed value object "0,0"',
  stack: 'Error\n' +
    '    at get_stack (evalmachine.<anonymous>:4551:21)\n' +
    '    at new AssertionError (evalmachine.<anonymous>:4544:22)\n' +
    '    at assert (evalmachine.<anonymous>:4528:19)\n' +
    '    at assert_not_equals (evalmachine.<anonymous>:1542:9)\n' +
    '    at assert_wrapper (evalmachine.<anonymous>:1445:30)\n' +
    '    at evalmachine.<anonymous>:122:13\n' +
    '    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)'
}
{
  status: 1,
  name: 'Check response clone use structureClone for teed ReadableStreams (Int32Arraychunk)',
  message: 'assert_not_equals: Buffer of cloned response stream is a clone of the original buffer got disallowed value object "0,0,0,0"',
  stack: 'Error\n' +
    '    at get_stack (evalmachine.<anonymous>:4551:21)\n' +
    '    at new AssertionError (evalmachine.<anonymous>:4544:22)\n' +
    '    at assert (evalmachine.<anonymous>:4528:19)\n' +
    '    at assert_not_equals (evalmachine.<anonymous>:1542:9)\n' +
    '    at assert_wrapper (evalmachine.<anonymous>:1445:30)\n' +
    '    at evalmachine.<anonymous>:122:13\n' +
    '    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)'
}
{
  status: 1,
  name: 'Check response clone use structureClone for teed ReadableStreams (Uint8Arraychunk)',
  message: 'assert_not_equals: Buffer of cloned response stream is a clone of the original buffer got disallowed value object "0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0"',
  stack: 'Error\n' +
    '    at get_stack (evalmachine.<anonymous>:4551:21)\n' +
    '    at new AssertionError (evalmachine.<anonymous>:4544:22)\n' +
    '    at assert (evalmachine.<anonymous>:4528:19)\n' +
    '    at assert_not_equals (evalmachine.<anonymous>:1542:9)\n' +
    '    at assert_wrapper (evalmachine.<anonymous>:1445:30)\n' +
    '    at evalmachine.<anonymous>:122:13\n' +
    '    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)'
}
{
  status: 1,
  name: 'Check response clone use structureClone for teed ReadableStreams (Uint8ClampedArraychunk)',
  message: 'assert_not_equals: Buffer of cloned response stream is a clone of the original buffer got disallowed value object "0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0"',
  stack: 'Error\n' +
    '    at get_stack (evalmachine.<anonymous>:4551:21)\n' +
    '    at new AssertionError (evalmachine.<anonymous>:4544:22)\n' +
    '    at assert (evalmachine.<anonymous>:4528:19)\n' +
    '    at assert_not_equals (evalmachine.<anonymous>:1542:9)\n' +
    '    at assert_wrapper (evalmachine.<anonymous>:1445:30)\n' +
    '    at evalmachine.<anonymous>:122:13\n' +
    '    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)'
}
{
  status: 1,
  name: 'Check response clone use structureClone for teed ReadableStreams (Uint16Arraychunk)',
  message: 'assert_not_equals: Buffer of cloned response stream is a clone of the original buffer got disallowed value object "0,0,0,0,0,0,0"',
  stack: 'Error\n' +
    '    at get_stack (evalmachine.<anonymous>:4551:21)\n' +
    '    at new AssertionError (evalmachine.<anonymous>:4544:22)\n' +
    '    at assert (evalmachine.<anonymous>:4528:19)\n' +
    '    at assert_not_equals (evalmachine.<anonymous>:1542:9)\n' +
    '    at assert_wrapper (evalmachine.<anonymous>:1445:30)\n' +
    '    at evalmachine.<anonymous>:122:13\n' +
    '    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)'
}
{
  status: 1,
  name: 'Check response clone use structureClone for teed ReadableStreams (Uint32Arraychunk)',
  message: 'assert_not_equals: Buffer of cloned response stream is a clone of the original buffer got disallowed value object "0,0,0,0"',
  stack: 'Error\n' +
    '    at get_stack (evalmachine.<anonymous>:4551:21)\n' +
    '    at new AssertionError (evalmachine.<anonymous>:4544:22)\n' +
    '    at assert (evalmachine.<anonymous>:4528:19)\n' +
    '    at assert_not_equals (evalmachine.<anonymous>:1542:9)\n' +
    '    at assert_wrapper (evalmachine.<anonymous>:1445:30)\n' +
    '    at evalmachine.<anonymous>:122:13\n' +
    '    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)'
}
{
  status: 1,
  name: 'Check response clone use structureClone for teed ReadableStreams (BigInt64Arraychunk)',
  message: 'assert_not_equals: Buffer of cloned response stream is a clone of the original buffer got disallowed value object "0,0"',
  stack: 'Error\n' +
    '    at get_stack (evalmachine.<anonymous>:4551:21)\n' +
    '    at new AssertionError (evalmachine.<anonymous>:4544:22)\n' +
    '    at assert (evalmachine.<anonymous>:4528:19)\n' +
    '    at assert_not_equals (evalmachine.<anonymous>:1542:9)\n' +
    '    at assert_wrapper (evalmachine.<anonymous>:1445:30)\n' +
    '    at evalmachine.<anonymous>:122:13\n' +
    '    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)'
}
{
  status: 1,
  name: 'Check response clone use structureClone for teed ReadableStreams (BigUint64Arraychunk)',
  message: 'assert_not_equals: Buffer of cloned response stream is a clone of the original buffer got disallowed value object "0,0"',
  stack: 'Error\n' +
    '    at get_stack (evalmachine.<anonymous>:4551:21)\n' +
    '    at new AssertionError (evalmachine.<anonymous>:4544:22)\n' +
    '    at assert (evalmachine.<anonymous>:4528:19)\n' +
    '    at assert_not_equals (evalmachine.<anonymous>:1542:9)\n' +
    '    at assert_wrapper (evalmachine.<anonymous>:1445:30)\n' +
    '    at evalmachine.<anonymous>:122:13\n' +
    '    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)'
}
{
  status: 1,
  name: 'Check response clone use structureClone for teed ReadableStreams (Float32Arraychunk)',
  message: 'assert_not_equals: Buffer of cloned response stream is a clone of the original buffer got disallowed value object "0,0,0,0"',
  stack: 'Error\n' +
    '    at get_stack (evalmachine.<anonymous>:4551:21)\n' +
    '    at new AssertionError (evalmachine.<anonymous>:4544:22)\n' +
    '    at assert (evalmachine.<anonymous>:4528:19)\n' +
    '    at assert_not_equals (evalmachine.<anonymous>:1542:9)\n' +
    '    at assert_wrapper (evalmachine.<anonymous>:1445:30)\n' +
    '    at evalmachine.<anonymous>:122:13\n' +
    '    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)'
}
{
  status: 1,
  name: 'Check response clone use structureClone for teed ReadableStreams (Float64Arraychunk)',
  message: 'assert_not_equals: Buffer of cloned response stream is a clone of the original buffer got disallowed value object "0,0"',
  stack: 'Error\n' +
    '    at get_stack (evalmachine.<anonymous>:4551:21)\n' +
    '    at new AssertionError (evalmachine.<anonymous>:4544:22)\n' +
    '    at assert (evalmachine.<anonymous>:4528:19)\n' +
    '    at assert_not_equals (evalmachine.<anonymous>:1542:9)\n' +
    '    at assert_wrapper (evalmachine.<anonymous>:1445:30)\n' +
    '    at evalmachine.<anonymous>:122:13\n' +
    '    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)'
}

mcollina · 2024-01-16T15:42:45Z

Ah right, I was mistaking it for something else related to this code. When the WPT runner fails a test, it doesn't cause the process to exst with an error (this was changed so it could run with node to report fetch test stats).

I think our CI should catch a regression on WPT coverage (cc @panva).

panva · 2024-01-16T15:51:02Z

Ah right, I was mistaking it for something else related to this code. When the WPT runner fails a test, it doesn't cause the process to exst with an error (this was changed so it could run with node to report fetch test stats).

I think our CI should catch a regression on WPT coverage (cc @panva).

No, undici's WPTs are ran in undici, not nodejs/node.

What runs in node is a daily WPT Report with the bundled undici version's git checkout and its own runner but this does not alert anyone on regressions given it also always pulls the latest WPT from web-platform-tests/wpt.

nodejs/citgm#982 would run undici's checkout of WPTs

We could also add a github actions job in nodejs/node to run undici's fetch and WPT tests on every PR.

panva · 2024-01-16T15:53:54Z

When the WPT runner fails a test, it doesn't cause the process to exst with an error (this was changed so it could run with node to report fetch test stats).

@KhafraDev I don't recall such a change, necessity. I believe exit codes are irrelevant for the nodejs daily WPT job. https://github.com/nodejs/node/blob/main/.github/workflows/daily-wpt-fyi.yml#L114

What's relevant is that the WPTs run and report, not how they exit.

KhafraDev · 2024-01-16T15:57:21Z

Thanks for the clarification, maybe the change was made in node core to ignore the exit code and in undici to always exit with 0? If it's being ignored in node it should be failing here, I'll look into it.

panva · 2024-01-16T16:25:48Z

FWIW process.exit(0) was introduced in #2226

weyert · 2024-01-17T00:44:26Z

Thank you. I have been trying to figure out how to make undici v6 work with jest and its jsdom environment but failed due to the use of structuredClone and wasn't able to stop structuredClone from being undefined in the tests.

Downgraded to v5 for now. Hope to be able to switch back to v6 when a new release with this fix is out :)

mcollina · 2024-01-17T09:53:52Z

We are not landing this, instead we are reverting the problem in Node.js core.

mcollina requested a review from KhafraDev January 16, 2024 11:41

Avoid structuredClone

46b64d1

Signed-off-by: Matteo Collina <[email protected]>

mcollina force-pushed the fix-2615 branch from 35c431c to 46b64d1 Compare January 16, 2024 11:44

mcollina mentioned this pull request Jan 16, 2024

Regression in Node.js v21.6.0 nodejs/node#51486

Closed

Fix tests for Node.js v20.11.0

54c7c7d

Signed-off-by: Matteo Collina <[email protected]>

mcollina requested a review from ronag January 16, 2024 12:01

mcollina mentioned this pull request Jan 16, 2024

lookup: re-add undici nodejs/citgm#982

Merged

2 tasks

ronag approved these changes Jan 16, 2024

View reviewed changes

KhafraDev requested changes Jan 16, 2024

View reviewed changes

metcoder95 approved these changes Jan 17, 2024

View reviewed changes

mcollina closed this Jan 17, 2024

Uzlopak deleted the fix-2615 branch February 21, 2024 22:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid structuredClone in Response() #2617

Avoid structuredClone in Response() #2617

mcollina commented Jan 16, 2024 •

edited

Loading

codecov-commenter commented Jan 16, 2024 •

edited

Loading

KhafraDev left a comment

mcollina commented Jan 16, 2024

mcollina commented Jan 16, 2024 •

edited

Loading

KhafraDev commented Jan 16, 2024 •

edited

Loading

mcollina commented Jan 16, 2024

panva commented Jan 16, 2024 •

edited

Loading

panva commented Jan 16, 2024 •

edited

Loading

KhafraDev commented Jan 16, 2024

panva commented Jan 16, 2024

weyert commented Jan 17, 2024

mcollina commented Jan 17, 2024

Avoid structuredClone in Response() #2617

Avoid structuredClone in Response() #2617

Conversation

mcollina commented Jan 16, 2024 • edited Loading

codecov-commenter commented Jan 16, 2024 • edited Loading

Codecov Report

KhafraDev left a comment

Choose a reason for hiding this comment

mcollina commented Jan 16, 2024

mcollina commented Jan 16, 2024 • edited Loading

KhafraDev commented Jan 16, 2024 • edited Loading

mcollina commented Jan 16, 2024

panva commented Jan 16, 2024 • edited Loading

panva commented Jan 16, 2024 • edited Loading

KhafraDev commented Jan 16, 2024

panva commented Jan 16, 2024

weyert commented Jan 17, 2024

mcollina commented Jan 17, 2024

mcollina commented Jan 16, 2024 •

edited

Loading

codecov-commenter commented Jan 16, 2024 •

edited

Loading

mcollina commented Jan 16, 2024 •

edited

Loading

KhafraDev commented Jan 16, 2024 •

edited

Loading

panva commented Jan 16, 2024 •

edited

Loading

panva commented Jan 16, 2024 •

edited

Loading