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(ext/node): avoid showing UNKNOWN error from TCP handle #25550

Merged
merged 3 commits into from
Sep 11, 2024

Conversation

kt3k
Copy link
Member

@kt3k kt3k commented Sep 10, 2024

This PR destroys Node.js stream with Deno's original error instead of UNKNOWN error from handle object.

In ext/node/polyfills/internal_binding/stream_wrap.ts, we try to simulate the system call error when reading from the internal Deno.Conn, however when it's used with tls.connect and the thrown error is a cert error (like InvalidData: invalid peer certificate: UnknownIssuer), then that error can't be mapped using this list of errors and it becomes UNKNOWN error.

This PR stops that mapping to UNKNOWN error (because it just obscures the actual reason and leads to the confusion like #20293), and instead destroys the stream with the Deno's original error (This is not ideal for the compatibility, but at least it's better than obscuring the error reason with 'UNKNOWN' error in my view).

related #20293

@kt3k kt3k added the ci-draft Run the CI on draft PRs. label Sep 10, 2024
@kt3k kt3k marked this pull request as ready for review September 10, 2024 09:24
@kt3k kt3k merged commit 200145a into denoland:main Sep 11, 2024
33 checks passed
@kt3k kt3k deleted the fix-unknown-error branch September 11, 2024 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-draft Run the CI on draft PRs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants