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): do not apply socket-init-workaround to ipc socket #27779

Merged
merged 11 commits into from
Jan 28, 2025

Conversation

kt3k
Copy link
Member

@kt3k kt3k commented Jan 22, 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

  • write test case

@kt3k kt3k added the ci-draft Run the CI on draft PRs. label Jan 22, 2025
Comment on lines +1 to +3
import { chromium } from "npm:playwright";
await chromium.launch();
console.log("chromium launched");
Copy link
Member

Choose a reason for hiding this comment

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

Does this actually work on CI?

Copy link
Member Author

@kt3k kt3k Jan 27, 2025

Choose a reason for hiding this comment

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

I suppose so. The first step in this test run -A npm:playwright install chromium should be downloading the chromium for the corresponding platform.

Copy link
Member

Choose a reason for hiding this comment

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

If it's not cached already, it's worth looking into caching the chromium artifact.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added browser binary cache at key playwright-${{ runner.os }}-${{ runner.arch }}

Looks it's been saved successfully

Screenshot 2025-01-27 at 17 25 25

https://github.com/denoland/deno/actions/runs/12984044432/job/36206262673

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for confirming

@kt3k kt3k force-pushed the fix-playwright-browser-launch branch from 6c67bbb to 2ef49be Compare January 27, 2025 03:59
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM!

@kt3k kt3k merged commit 0e47205 into denoland:main Jan 28, 2025
17 checks passed
@kt3k kt3k deleted the fix-playwright-browser-launch branch January 28, 2025 06:38
@nshiab
Copy link

nshiab commented Jan 28, 2025

Do you know when this will be part of an official release? Thank you!

@bartlomieju
Copy link
Member

In 2.2 next week. You can upgrade to canary in the meantime (deno upgrade --canary)

@nshiab
Copy link

nshiab commented Jan 28, 2025

Amazing! Thanks!

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
Labels
ci-draft Run the CI on draft PRs.
Projects
None yet
4 participants