fix(ext/node): tls.connect regression #27707
Merged
Conversation
This file contains 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
kt3k
force-pushed
the
fix-tls-connect-regression
branch
from
6c94ccd to
6f228fc
Compare
littledivy
approved these changes
Jan 17, 2025
bartlomieju
pushed a commit
to bartlomieju/deno
that referenced
this pull request
Jan 17, 2025
The TLS start sequence has been broken since denoland#26661 because of the way how we wrap TCP handle to create TLS handle. denoland#26661 introduced happy-eyeballs algorithm and some connection could be dropped because of happy-eyeball attempt timeout. The current implementation doesn't consider that case and it could start TLS handshake with timed out TCP connection. That caused denoland#27652 . This PR fixes it by changing the initialization steps. Now `wrapHandle` of TLSSocket set up `afterConnectTls` callback in TCP handle, and `afterConnect` of TCP handle calls it at `connect` event timing if it exists. This avoids starting TLS session with timed out connection. closes denoland#27652
crowlKats
pushed a commit
that referenced
this pull request
Jan 21, 2025
The TLS start sequence has been broken since #26661 because of the way how we wrap TCP handle to create TLS handle. #26661 introduced happy-eyeballs algorithm and some connection could be dropped because of happy-eyeball attempt timeout. The current implementation doesn't consider that case and it could start TLS handshake with timed out TCP connection. That caused #27652 . This PR fixes it by changing the initialization steps. Now `wrapHandle` of TLSSocket set up `afterConnectTls` callback in TCP handle, and `afterConnect` of TCP handle calls it at `connect` event timing if it exists. This avoids starting TLS session with timed out connection. closes #27652 (cherry picked from commit b55451b)
This was referenced Jan 23, 2025
kt3k
added a commit
that referenced
this pull request
Jan 28, 2025
) This PR resolves 2 issues of Socket class of node compat (both are related to playwright) Currently `browser.launch()` of playwright is not working. `browser.launch` opens PipeTransport (which is based on Pipe/IPC socket) with the browser process. But that pipe doesn't start reading the data because of the workaround #27662 (which pauses the socket at the beginning if it's from playwright-core). This PR fixes this issue by checking whether the given handle is `ipc` handle or not. Another issue is that sock-init-workaround for TLS connection stopped working at #27707 because of the changes of TLS socket initialization steps. This change fixes the issue by correctly returning the function in workaround path. The added case `specs::npm::playwright_compat` checks both fixes with actual playwright and playwright-core packages. `browser.launch` issues closes #16899 closes #27623 `https.request` issue closes #27658
bartlomieju
pushed a commit
that referenced
this pull request
Jan 30, 2025
) This PR resolves 2 issues of Socket class of node compat (both are related to playwright) Currently `browser.launch()` of playwright is not working. `browser.launch` opens PipeTransport (which is based on Pipe/IPC socket) with the browser process. But that pipe doesn't start reading the data because of the workaround #27662 (which pauses the socket at the beginning if it's from playwright-core). This PR fixes this issue by checking whether the given handle is `ipc` handle or not. Another issue is that sock-init-workaround for TLS connection stopped working at #27707 because of the changes of TLS socket initialization steps. This change fixes the issue by correctly returning the function in workaround path. The added case `specs::npm::playwright_compat` checks both fixes with actual playwright and playwright-core packages. `browser.launch` issues closes #16899 closes #27623 `https.request` issue closes #27658
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.
The TLS start sequence has been broken since #26661 because of the way how we wrap TCP handle to create TLS handle.
#26661 introduced happy-eyeballs algorithm and some connection could be dropped because of happy-eyeball attempt timeout. The current implementation doesn't consider that case and it could start TLS handshake with timed out TCP connection. That caused #27652 .
This PR fixes it by changing the initialization steps. Now
wrapHandleof TLSSocket set upafterConnectTlscallback in TCP handle, andafterConnectof TCP handle calls it atconnectevent timing if it exists. This avoids starting TLS session with timed out connection.closes #27652