Skip to content

Commit

Permalink
fix(ext/node): fix playwright http client (#27662)
Browse files Browse the repository at this point in the history
  • Loading branch information
kt3k authored Jan 14, 2025
1 parent 0b03314 commit c943f56
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 11 deletions.
2 changes: 1 addition & 1 deletion ext/node/polyfills/_tls_wrap.ts
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ export class TLSSocket extends net.Socket {
const afterConnect = handle.afterConnect;
handle.afterConnect = async (req: any, status: number) => {
options.hostname ??= undefined; // coerce to undefined if null, startTls expects hostname to be undefined
if (tlssock._isNpmAgent) {
if (tlssock._needsSockInitWorkaround) {
// skips the TLS handshake for @npmcli/agent as it's handled by
// onSocket handler of ClientRequest object.
tlssock.emit("secure");
Expand Down
28 changes: 18 additions & 10 deletions ext/node/polyfills/net.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1157,6 +1157,13 @@ function _emitCloseNT(s: Socket | Server) {
s.emit("close");
}

// The packages that need socket initialization workaround
const pkgsNeedsSockInitWorkaround = [
"@npmcli/agent",
"npm-check-updates",
"playwright-core",
];

/**
* This class is an abstraction of a TCP socket or a streaming `IPC` endpoint
* (uses named pipes on Windows, and Unix domain sockets otherwise). It is also
Expand Down Expand Up @@ -1201,9 +1208,11 @@ export class Socket extends Duplex {
_host: string | null = null;
// deno-lint-ignore no-explicit-any
_parent: any = null;
// The flag for detecting if it's called in @npmcli/agent
// Skip some initialization (initial read and tls handshake if it's tls socket).
// If this flag is true, it's used as connection for http(s) request, and
// the reading and TLS handshake is done by the http client.
// See discussions in https://github.com/denoland/deno/pull/25470 for more details.
_isNpmAgent = false;
_needsSockInitWorkaround = false;
autoSelectFamilyAttemptedAddresses: AddressInfo[] | undefined = undefined;

constructor(options: SocketOptions | number) {
Expand All @@ -1224,21 +1233,20 @@ export class Socket extends Duplex {

super(options);

// Note: If the socket is created from one of:
// - @npmcli/agent
// - npm-check-updates (bundles @npmcli/agent as a dependency)
// Note: If the socket is created from one of `pkgNeedsSockInitWorkaround`,
// the 'socket' event on ClientRequest object happens after 'connect' event on Socket object.
// That swaps the sequence of op_node_http_request_with_conn() call and
// initial socket read. That causes op_node_http_request_with_conn() not
// working.
// To avoid the above situation, we detect the socket created from
// @npmcli/agent and pause the socket (and also skips the startTls call
// if it's TLSSocket)
// one of those packages using stack trace and pause the socket
// (and also skips the startTls call if it's TLSSocket)
// TODO(kt3k): Remove this workaround
const errorStack = new Error().stack;
this._isNpmAgent = errorStack?.includes("@npmcli/agent") ||
errorStack?.includes("npm-check-updates") || false;
if (this._isNpmAgent) {
this._needsSockInitWorkaround = pkgsNeedsSockInitWorkaround.some((pkg) =>
errorStack?.includes(pkg)
);
if (this._needsSockInitWorkaround) {
this.pause();
}

Expand Down

0 comments on commit c943f56

Please sign in to comment.