[Feature]: Fix Leaking async ops in Deno

[guillaume86]

Feature description

Running on Deno with a npm: import is almost perfect but there's still some issues:

• There's a warning Warning: Not implemented: ClientRequest.options.createConnection which doesn't seem to break functionnality.
• When the app exits, there's a 30s delay

You can expose these issues with a simple test file:

import puppeteer from "npm:[email protected]";

Deno.test("A", async (t) => {
  const browser = await puppeteer.launch({
    headless: "new",
    handleSIGHUP: Deno.build.os !== "windows",
  });

  try {
    await t.step("B", async () => {});
  } finally {
    await browser.close();
  }
});

save as ´main_test.ts´ and run it with deno test -A --trace-ops, you will see this:

Check file:///app/main_test.ts
running 1 test from ./main_test.ts
A ...
------- output -------
Warning: Not implemented: ClientRequest.options.createConnection
----- output end -----
  B ... ok (1ms)
A ... FAILED (266ms)

 ERRORS 

A => ./main_test.ts:4:6
error: Leaking async ops:
  - 1 async operation to op_read was started in this test, but never completed. The operation was started here:
    at handleOpCallTracing (ext:core/00_infra.js:125:42)
    at Object.op_read (ext:core/00_infra.js:302:21)
    at TcpConn.read (ext:deno_net/01_net.js:137:26)
    at TCP.#read (ext:deno_node/internal_binding/stream_wrap.ts:222:44)
    at TCP.#read (ext:deno_node/internal_binding/stream_wrap.ts:250:17)
    at eventLoopTick (ext:core/01_core.js:64:7)
  - 1 async operation to sleep for a duration was started in this test, but never completed. This is often caused by not cancelling a `setTimeout` or `setInterval` call. The operation was started here:     
    at handleOpCallTracing (ext:core/00_infra.js:125:42)
    at op_sleep (ext:core/00_infra.js:302:21)
    at runAfterTimeout (ext:deno_web/02_timers.js:234:20)
    at initializeTimer (ext:deno_web/02_timers.js:192:3)
    at setTimeout (ext:deno_web/02_timers.js:336:10)
    at Timeout.<computed> (ext:deno_node/internal/timers.mjs:67:7)
    at new Timeout (ext:deno_node/internal/timers.mjs:54:37)
    at setTimeout (node:timers:16:10)
    at WebSocket.close (file:///deno-dir/npm/registry.npmjs.org/ws/8.13.0/lib/websocket.js:321:24)
    at NodeWebSocketTransport.close (file:///deno-dir/npm/registry.npmjs.org/puppeteer-core/20.0.0/lib/esm/puppeteer/common/NodeWebSocketTransport.js:71:71)

 FAILURES 

A => ./src/main_test.ts:4:6

FAILED | 0 passed (1 step) | 1 failed (269ms)

error: Test failed

The async leaks are the source of the 30s delay on close.
The issues seem to originate from the ws dependency, because just monkey patching NodeWebSocketTransport to use the standard Deno WebSocket client seem to fix both issues:

import { NodeWebSocketTransport } from "npm:[email protected]/lib/esm/puppeteer/node/NodeWebSocketTransport.js";
import puppeteer from "npm:[email protected]";
export default puppeteer;

// monkey patching the NodeWebSocketTransport class to use the native WebSocket
// implementation in Deno instead of the one from the `ws` package which causes issues
NodeWebSocketTransport.create = function create(url: string) {
  return new Promise((resolve, reject) => {
    const ws = new WebSocket(url, []);
    ws.addEventListener("open", () => {
      return resolve(new NodeWebSocketTransport(ws));
    });
    ws.addEventListener("error", reject);
  });
};

I'm not sure it matters but the deno WebSocket class do not allow options (headers, etc), in my testing it didn't seem to impact functionality.

Would you consider adding a deno entrypoint in your project which would substitute the ws WebSocket with the globalThis one (something like https://github.com/yargs/yargs/blob/main/deno.ts) ? Or maybe just a setWebSocketFactory(...) so we can cleanly replace the WebSocket class.

[OrKoN]

@guillaume86 I am not familiar with deno but it would great to get things running there. Does it make sense to address the issue in the ws module? (as I understood it, the root cause is there). Also, we do allow the user's to define headers for the web socket request and we would need to keep supporting that (see headers in https://pptr.dev/api/puppeteer.connectoptions/).

[guillaume86]

Hi, thanks for answering.

Yes I'm aware of the headers option in puppeteer, unfortunately Deno do not look like it will implement them as it's going against the Web spec: denoland/deno#7632 .

As for fixing the ws package for Deno it's probably above my skill level (and the amount of time I'm willing to invest in this tbh). I'm afraid an issue there will not get any traction since for most use cases the standard Deno class is enough.

I'm not sure what are the use cases for custom headers in puppeteer, if it's nothing major, a deno specific entrypoint with a warning that custom headers are not supported would be nice. Otherwise a webSocketFactory option seem like the minimum changes option, let users worry about supporting custom headers or not.

Edit: websockets/ws#1886 (comment)

[franz101]

Hi, thanks for answering.

Yes I'm aware of the headers option in puppeteer, unfortunately Deno do not look like it will implement them as it's going against the Web spec: denoland/deno#7632 .

As for fixing the ws package for Deno it's probably above my skill level (and the amount of time I'm willing to invest in this tbh). I'm afraid an issue there will not get any traction since for most use cases the standard Deno class is enough.

I'm not sure what are the use cases for custom headers in puppeteer, if it's nothing major, a deno specific entrypoint with a warning that custom headers are not supported would be nice. Otherwise a webSocketFactory option seem like the minimum changes option, let users worry about supporting custom headers or not.

Edit: websockets/ws#1886 (comment)

they have a websocketstream api that supports headers

[Lightning00Blade]

There seems to have been an upstream fix for this, which landed with Deno v1.46.3, I will close this now, but if the issue is still there please commend and I will reopen it.