fix(ext/node): remove kUseNativeWrap on TLS TCP handles to fix HTTPS regression#33251
Open
rawkakani wants to merge 1 commit intodenoland:mainfrom
Open
fix(ext/node): remove kUseNativeWrap on TLS TCP handles to fix HTTPS regression#33251rawkakani wants to merge 1 commit intodenoland:mainfrom
rawkakani wants to merge 1 commit intodenoland:mainfrom
Conversation
8a3671f to
53900b6
Compare
…regression The native wrap path introduced in denoland#33184 sets `kUseNativeWrap = true` on freshly created TCP handles in `TLSSocket._wrapHandle`. This forces the handle into `#nativeConnect` which never populates `kStreamBaseField`. When `http.ts:_writeHeader` later reads `handle[kStreamBaseField][internalRidSymbol]` to obtain the connection rid for `op_node_http_request_with_conn`, it crashes: TypeError: Cannot read properties of undefined (reading 'Symbol(Deno.internal.rid)') This breaks all HTTPS client requests via Node compat (Playwright, @deno/sandbox, any npm package using https.request). Regression bisected: - 2.7.11: works - 2.7.12: broken Fix: remove the `kUseNativeWrap = true` assignment so freshly created TCP handles use the legacy connect path that populates `kStreamBaseField` with a `TcpConn` carrying a valid rid. Fixes denoland#33231 Fixes denoland#33229
53900b6 to
4ffa0b4
Compare
Member
|
Thanks, but this needs a test. I have doubts that this actually fixes the problem |
Author
understood, shall let the ai know 😅 |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Removes
handle[kUseNativeWrap] = truefromTLSSocket._wrapHandlein_tls_wrap.js, fixing an HTTPS client regression introduced in v2.7.12.AI Disclosure
This PR was written with assistance from Claude (Anthropic). This PR was fully authored with Claude Code (Anthropic). Claude performed the version bisection testing, identified the breaking commit by diffing v2.7.11 and v2.7.12, traced the root cause through the source code, wrote the fix, and drafted the commit message and PR description.
Problem
PR #33184 (
3ec37cc7e012) addedkUseNativeWrap = trueon freshly created TCP handles inside_wrapHandle. This forces the handle into the#nativeConnectpath which never populateskStreamBaseField.When
http.ts:_writeHeaderlater readshandle[kStreamBaseField][internalRidSymbol]to get the connection rid forop_node_http_request_with_conn, it crashes:This breaks all HTTPS client requests via Node compat — affects Playwright (#33229),
@deno/sandbox, and any npm package usinghttps.request().Regression bisection
Fix
Remove the
kUseNativeWrap = trueassignment (1 line) so freshly created TCP handles use the legacy connect path that populateskStreamBaseFieldwith aTcpConncarrying a valid rid.The native
TLSWrapstill works — it just wraps a handle that went through the legacy connect path.Reproduction
Test plan
The existing Node compat test suite for
httpsandtlscovers this path. A minimal test:Fixes #33231
Fixes #33229