fix(#3736): back-port 183f8e9 to v6.x by ggoodman · Pull Request #3855 · nodejs/undici (original) (raw)

This relates to...

This applies the changes in #3740 that failed automatic backport into v6.x. Our testing suggests that this resolves #3848.

Rationale

Changes

Adds a no-op "error" event handler to the res stream when handling AbortSignal aborts in request() that was otherwise leading to process-level uncaught exceptions in our codebase.

Features

N/A

Bug Fixes

Fixes #3736 in v6.x
Fixes #3848 in v6.x

Breaking Changes and Deprecations

Status

I have read and agreed to the Developer's Certificate of Origin
Tested - Note that test/issue-3356.js times out with this change under npm test but not if run directly via node test/issue-3356.js.
[S] Benchmarked (optional)
[S] Documented
Review ready
In review
Merge ready

Test failures seem limited to Node 22 and TLS behaviour, not the substance of the PR:

test at test/http2.js:310:1
✖ [v20] Request should fail if allowH2 is false and server advertises h1 only (6.669102ms)
  AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
  + actual - expected
  
  + '809803CB687F0000:error:0A000460:SSL routines:ssl3_read_bytes:tlsv1 alert no application protocol:../deps/openssl/openssl/ssl/record/rec_layer_s3.c:1605:SSL alert number 120\n'
  - 'Client network socket disconnected before secure TLS connection was established'
      at res.<computed> [as strictEqual] (/home/runner/work/undici/undici/node_modules/@matteo.collina/tspl/tspl.js:52:35)
      at TestContext.<anonymous> (/home/runner/work/undici/undici/test/http2.js:348:9)
      at process.processTicksAndRejections (node:internal/process/task_queues:105:5)
      at async Test.run (node:internal/test_runner/test:935:9)
      at async Test.processPendingSubtests (node:internal/test_runner/test:633:7) {
    generatedMessage: true,
    code: 'ERR_ASSERTION',
    actual: '809803CB687F0000:error:0A000460:SSL routines:ssl3_read_bytes:tlsv1 alert no application protocol:../deps/openssl/openssl/ssl/record/rec_layer_s3.c:1605:SSL alert number 120\n',
    expected: 'Client network socket disconnected before secure TLS connection was established',
    operator: 'strictEqual'
  }

OK I found #3769 which addresses a change change in error messages in Node v22. I backported the fix and hope tests now pass.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The user should have an error handler registered...

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure I agree with this but it seems I approved the other one...

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})