From 54eb0ead3d22483e93a0da606d50ff0c0072dd34 Mon Sep 17 00:00:00 2001 From: Luca Casonato Date: Fri, 15 Mar 2024 11:43:24 +0100 Subject: [PATCH 1/2] Re-enable `unref` in Deno This re-enables unref-by-default in Deno, and fixes the event loop starvation issue by explicitly re-ref'ing the esbuild sub-process when a user calls `stop()`. --- CHANGELOG.md | 12 ++++++++++++ lib/deno/mod.ts | 29 ++++++++++++++++++++++++----- lib/shared/types.ts | 19 +++++++++++-------- 3 files changed, 47 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 70f6e8da7c..80cb7096e5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,17 @@ # Changelog +## 0.20.3 + +* Re-enable `unref` behaviour for Deno + + Version 0.20.0 of esbuild changed how the esbuild child process is run in esbuild's API for Deno. Previously it used `Deno.run` but that API is being removed in favor of `Deno.Command`. As part of this change, esbuild is now calling the new `unref` function on esbuild's long-lived child process, which is supposed to allow Deno to exit when your code has finished running even though the child process is still around (previously you had to explicitly call esbuild's `stop()` function to terminate the child process for Deno to be able to exit). + + In version 0.20.2 of esbuild, this change was reverted because it was discovered that this change could result in the Deno process exiting when with an error with message `error: Promise resolution is still pending but the event loop has already resolved`, if the `stop()` function was called when there were no other ref'd pending async operations scheduled in the process. + + It has now been determined that this was caused by a bug in the implementation of esbuild's `stop()` function. This function did not `ref` the child process again before waiting for it to exit. This meant that when a user explicitly called `stop()`, a promise was returned that was backed only by `unref`'d async operations. If the user then awaited this promise with no other operations pending, the event loop would starve, and the process would exit with the error message seen above. This has been fixed by re-`ref`ing the child process before waiting for it to exit. + + This version of esbuild fixes this bug, and returns the `unref` behaviour to the behaviour present in esbuild's 0.20.0 release. + ## 0.20.2 * Support TypeScript experimental decorators on `abstract` class fields ([#3684](https://github.com/evanw/esbuild/issues/3684)) diff --git a/lib/deno/mod.ts b/lib/deno/mod.ts index b623392936..425f434805 100644 --- a/lib/deno/mod.ts +++ b/lib/deno/mod.ts @@ -192,6 +192,8 @@ type SpawnFn = (cmd: string, options: { read(): Promise close(): Promise | void status(): Promise<{ code: number }> + unref(): void + ref(): void } // Deno ≥1.40 @@ -209,6 +211,11 @@ const spawnNew: SpawnFn = (cmd, { args, stdin, stdout, stderr }) => { write: bytes => writer.write(bytes), read: () => reader.read().then(x => x.value || null), close: async () => { + // Ref the child process again, so that a user calling `close()` can await + // the returned promise without the event loop starving because there are + // no more ref'd async tasks. + child.ref() + // We can't call "kill()" because it doesn't seem to work. Tests will // still fail with "A child process was opened during the test, but not // closed during the test" even though we kill the child process. @@ -233,6 +240,8 @@ const spawnNew: SpawnFn = (cmd, { args, stdin, stdout, stderr }) => { await child.status }, status: () => child.status, + unref: () => child.unref(), + ref: () => child.ref(), } } @@ -273,6 +282,8 @@ const spawnOld: SpawnFn = (cmd, { args, stdin, stdout, stderr }) => { child.close() }, status: () => child.status(), + unref: () => { }, + ref: () => { }, } } @@ -328,12 +339,20 @@ const ensureServiceIsRunning = (): Promise => { }) readMoreStdout() + let refCount = 0 + child.unref() // Allow Deno to exit when esbuild is running + + const refs: common.Refs = { + ref() { if (++refCount === 1) child.ref(); }, + unref() { if (--refCount === 0) child.unref(); }, + } + return { build: (options: types.BuildOptions) => new Promise((resolve, reject) => { service.buildOrContext({ callName: 'build', - refs: null, + refs, options, isTTY, defaultWD, @@ -345,7 +364,7 @@ const ensureServiceIsRunning = (): Promise => { new Promise((resolve, reject) => service.buildOrContext({ callName: 'context', - refs: null, + refs, options, isTTY, defaultWD, @@ -356,7 +375,7 @@ const ensureServiceIsRunning = (): Promise => { new Promise((resolve, reject) => service.transform({ callName: 'transform', - refs: null, + refs, input, options: options || {}, isTTY, @@ -389,7 +408,7 @@ const ensureServiceIsRunning = (): Promise => { new Promise((resolve, reject) => service.formatMessages({ callName: 'formatMessages', - refs: null, + refs, messages, options, callback: (err, res) => err ? reject(err) : resolve(res!), @@ -399,7 +418,7 @@ const ensureServiceIsRunning = (): Promise => { new Promise((resolve, reject) => service.analyzeMetafile({ callName: 'analyzeMetafile', - refs: null, + refs, metafile: typeof metafile === 'string' ? metafile : JSON.stringify(metafile), options, callback: (err, res) => err ? reject(err) : resolve(res!), diff --git a/lib/shared/types.ts b/lib/shared/types.ts index df6482ecf9..28ceeb9b13 100644 --- a/lib/shared/types.ts +++ b/lib/shared/types.ts @@ -664,20 +664,23 @@ export let version: string // Call this function to terminate esbuild's child process. The child process // is not terminated and re-created after each API call because it's more -// efficient to keep it around when there are multiple API calls. +// efficient to keep it around when there are multiple API calls. This child +// process normally exits automatically when the parent process exits, so you +// usually don't need to call this function. // -// In node this happens automatically before the parent node process exits. So -// you only need to call this if you know you will not make any more esbuild -// API calls and you want to clean up resources. -// -// Unlike node, Deno lacks the necessary APIs to clean up child processes -// automatically. You must manually call stop() in Deno when you're done -// using esbuild or Deno will continue running forever. +// One reason you might want to call this is if you know you will not make any +// more esbuild API calls and you want to clean up resources (since the esbuild +// child process takes up some memory even when idle). // // Another reason you might want to call this is if you are using esbuild from // within a Deno test. Deno fails tests that create a child process without // killing it before the test ends, so you have to call this function (and // await the returned promise) in every Deno test that uses esbuild. +// +// You may also start esbuild once at the top level of your test suite instead +// of starting and stopping the esbuild process for every test. This will not +// interfere with the resource sanitizer, and will improve the efficiency of +// your tests because the esbuild process can be reused between tests. export declare function stop(): Promise // Note: These declarations exist to avoid type errors when you omit "dom" from From 40f893a7a071709458b9434c8eeb9d9e84c3004b Mon Sep 17 00:00:00 2001 From: Luca Casonato Date: Fri, 15 Mar 2024 11:48:41 +0100 Subject: [PATCH 2/2] more precise comment --- CHANGELOG.md | 2 +- lib/shared/types.ts | 11 ++++++----- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 80cb7096e5..8533950c64 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ ## 0.20.3 -* Re-enable `unref` behaviour for Deno +* Re-enable `unref` behaviour for Deno ([#3701](https://github.com/evanw/esbuild/issues/3701)) Version 0.20.0 of esbuild changed how the esbuild child process is run in esbuild's API for Deno. Previously it used `Deno.run` but that API is being removed in favor of `Deno.Command`. As part of this change, esbuild is now calling the new `unref` function on esbuild's long-lived child process, which is supposed to allow Deno to exit when your code has finished running even though the child process is still around (previously you had to explicitly call esbuild's `stop()` function to terminate the child process for Deno to be able to exit). diff --git a/lib/shared/types.ts b/lib/shared/types.ts index 28ceeb9b13..31f4e6f504 100644 --- a/lib/shared/types.ts +++ b/lib/shared/types.ts @@ -675,12 +675,13 @@ export let version: string // Another reason you might want to call this is if you are using esbuild from // within a Deno test. Deno fails tests that create a child process without // killing it before the test ends, so you have to call this function (and -// await the returned promise) in every Deno test that uses esbuild. +// await the returned promise) in every Deno test that starts esbuild. // -// You may also start esbuild once at the top level of your test suite instead -// of starting and stopping the esbuild process for every test. This will not -// interfere with the resource sanitizer, and will improve the efficiency of -// your tests because the esbuild process can be reused between tests. +// You may also start esbuild once at the top level of your test suite (by +// calling `initialize()`) instead of starting and stopping the esbuild process +// for every test. This will not interfere with the resource sanitizer, and will +// improve the efficiency of your tests because the esbuild process can be +// reused between tests. export declare function stop(): Promise // Note: These declarations exist to avoid type errors when you omit "dom" from