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

Re-enable unref in Deno #3701

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -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))
Expand Down
29 changes: 24 additions & 5 deletions lib/deno/mod.ts
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,8 @@ type SpawnFn = (cmd: string, options: {
read(): Promise<Uint8Array | null>
close(): Promise<void> | void
status(): Promise<{ code: number }>
unref(): void
ref(): void
}

// Deno ≥1.40
Expand All @@ -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()
Comment on lines +214 to +217
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only new code. All other code is just a revert of 116f63e.


// 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.
Expand All @@ -233,6 +240,8 @@ const spawnNew: SpawnFn = (cmd, { args, stdin, stdout, stderr }) => {
await child.status
},
status: () => child.status,
unref: () => child.unref(),
ref: () => child.ref(),
}
}

Expand Down Expand Up @@ -273,6 +282,8 @@ const spawnOld: SpawnFn = (cmd, { args, stdin, stdout, stderr }) => {
child.close()
},
status: () => child.status(),
unref: () => { },
ref: () => { },
}
}

Expand Down Expand Up @@ -328,12 +339,20 @@ const ensureServiceIsRunning = (): Promise<Service> => {
})
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<types.BuildResult>((resolve, reject) => {
service.buildOrContext({
callName: 'build',
refs: null,
refs,
options,
isTTY,
defaultWD,
Expand All @@ -345,7 +364,7 @@ const ensureServiceIsRunning = (): Promise<Service> => {
new Promise<types.BuildContext>((resolve, reject) =>
service.buildOrContext({
callName: 'context',
refs: null,
refs,
options,
isTTY,
defaultWD,
Expand All @@ -356,7 +375,7 @@ const ensureServiceIsRunning = (): Promise<Service> => {
new Promise<types.TransformResult>((resolve, reject) =>
service.transform({
callName: 'transform',
refs: null,
refs,
input,
options: options || {},
isTTY,
Expand Down Expand Up @@ -389,7 +408,7 @@ const ensureServiceIsRunning = (): Promise<Service> => {
new Promise((resolve, reject) =>
service.formatMessages({
callName: 'formatMessages',
refs: null,
refs,
messages,
options,
callback: (err, res) => err ? reject(err) : resolve(res!),
Expand All @@ -399,7 +418,7 @@ const ensureServiceIsRunning = (): Promise<Service> => {
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!),
Expand Down
19 changes: 11 additions & 8 deletions lib/shared/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<void>

// Note: These declarations exist to avoid type errors when you omit "dom" from
Expand Down
Loading