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

fix(#3736): back-port 183f8e9 to v6.x #3855

Merged
merged 2 commits into from
Nov 21, 2024
Merged

Conversation

ggoodman
Copy link
Contributor

@ggoodman ggoodman commented Nov 20, 2024

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

@ggoodman
Copy link
Contributor Author

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

@ggoodman
Copy link
Contributor Author

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

Copy link
Member

@ronag ronag left a comment

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...

Copy link
Member

@ronag ronag left a comment

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...

@ronag ronag merged commit 353ab63 into nodejs:v6.x Nov 21, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants