esbuild inside Deno.test() leaks resources

[AlexJeffcott]

When using esbuild inside a Deno.test() esbuild leaks at least one pending resource which Deno reports as a test error. Calling esbuild's await stop() does not work.

Deno version: 1.41.1 (latest)

esbuild version: 0.21.1 (latest)

A minimal reproducible example is this test which I expect to pass:

#!/usr/bin/env -S deno test -A

import { assertEquals } from 'https://deno.land/std@0.218.0/assert/mod.ts'
import * as esbuild from 'https://deno.land/x/esbuild@v0.20.1/mod.js'

Deno.test('should transform', async () => {
  const result = await esbuild.transform('const test: boolean = true', { loader: 'ts' })
  assertEquals(result.code, 'const test = true;\n')
  await esbuild.stop()
})

Deno.test('should transform again', async () => {
  const result = await esbuild.transform('const test: boolean = true', { loader: 'ts' })
  assertEquals(result.code, 'const test = true;\n')
  await esbuild.stop()
})

Will output:

error: Promise resolution is still pending but the event loop has already resolved.

The same problem applies to the npm: version, albeit intermittently:

#!/usr/bin/env -S deno test -A

import { assertEquals } from 'https://deno.land/std@0.218.0/assert/mod.ts'
import * as esbuild from 'npm:esbuild@0.20.1'

Deno.test('should transform', async () => {
  const result = await esbuild.transform('const test: boolean = true', { loader: 'ts' })
  assertEquals(result.code, 'const test = true;\n')
  await esbuild.stop()
})

Deno.test('should transform again', async () => {
  const result = await esbuild.transform('const test: boolean = true', { loader: 'ts' })
  assertEquals(result.code, 'const test = true;\n')
  await esbuild.stop()
})

Will output 90% of the time:

error: Leaks detected:
  - A child process was started during the test, but not closed during the test. Close the child process by calling `proc.kill()` or `proc.close()`.
  - An async operation to wait for a subprocess to exit was started in this test, but never completed. This is often caused by not awaiting the result of a `Deno.Process#status` call.
To get more details where leaks occurred, run again with the --trace-leaks flag.

[evanw]

It looks like this isn't caught by esbuild's current tests because esbuild's tests do something like this, which for some reason doesn't cause a problem with Deno:

import { assertEquals } from 'https://deno.land/std@0.218.0/assert/mod.ts'
import * as esbuild from 'https://deno.land/x/esbuild@v0.20.1/mod.js'

const withTimeout = async (fn) => new Promise((resolve, reject) => {
  const minutes = 5
  const timeout = setTimeout(() => reject(new Error(`Timeout for "${name}" after ${minutes} minutes`)), minutes * 60 * 1000)
  const cancel = () => clearTimeout(timeout)
  const promise = fn()
  promise.then(cancel, cancel)
  promise.then(resolve, reject)
})

Deno.test('should transform', () => withTimeout(async () => {
  const result = await esbuild.transform('const test: boolean = true', { loader: 'ts' })
  assertEquals(result.code, 'const test = true;\n')
  await esbuild.stop()
}))

Deno.test('should transform again', () => withTimeout(async () => {
  const result = await esbuild.transform('const test: boolean = true', { loader: 'ts' })
  assertEquals(result.code, 'const test = true;\n')
  await esbuild.stop()
}))

I think the problem is perhaps that esbuild's API calls Deno's unref function on the child process, which is supposed to allow Deno to exit while esbuild is still running. This is fine in Node, but perhaps Deno's unref works differently than Node's unref. I'm doing this because someone on the Deno core team recommended doing that. It seems like I shouldn't be doing this though.

I can remove the call to unref, which will bring back the requirement to call esbuild.stop() for Deno to be able to exit when using esbuild.

[lucacasonato]

@evanw Sorry for not responding earlier.

I just analyzed what the issue is, and I do not think Deno is doing anything wrong here. The problem is that await esbuild.stop() is waiting exclusively on unref'd resources, which means that to the Deno process, it looks like there is a promise at the top level (or in a test), that has no path to getting resolved (because there is no pending asynchronous tasks that are holding up the event loop from exiting).

You can actually also trivially see this issue by writing some code top-level:

import * as esbuild from "https://deno.land/x/esbuild@v0.20.0/mod.js";

const res = await esbuild.build({
  entryPoints: ["./demo.ts"],
  write: false,
  format: "esm",
  platform: "neutral",
});
console.log(res.outputFiles[0].text);
await esbuild.stop();

This will fail with:

function add(a, b) {
  return a + b;
}
export {
  add
};

error: Top-level await promise never resolved
await esbuild.stop();
^
    at <anonymous> (file:///mnt/artemis/Projects/github.com/lucacasonato/esbuild_deno_loader/test.ts:10:1)

If you were to implement a stop() method in the Node version of esbuild also, that did not re-ref the process before waiting on exit, you would run into the exact same issue.

The fix is simple, and works just like the reference-counted ref() and unref() that you already do while a build(), transform() or similar is ongoing: you have to re-ref the subprocess before returning the stop() promise to the user. Otherwise this promise is backed entirely by unref'd async ops, which will cause the event loop to starve. I've opened a PR to re-enable the unref behaviour with a fix: #3701