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

work around api deprecations in deno 1.40.x (#3609) #3611

Merged
merged 8 commits into from
Jan 27, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
42 changes: 36 additions & 6 deletions .github/workflows/ci.yml
Expand Up @@ -89,12 +89,10 @@ jobs:
with:
node-version: 16

# The version of Deno is pinned because version 1.25.1 was causing test
# flakes due to random segfaults.
- name: Setup Deno 1.24.0
- name: Setup Deno 1.40.0
uses: denoland/setup-deno@main
with:
deno-version: v1.24.0
deno-version: v1.40.0

- name: Check out code into the Go module directory
uses: actions/checkout@v3
Expand Down Expand Up @@ -199,8 +197,8 @@ jobs:

make test-yarnpnp

esbuild-old-versions:
name: esbuild CI (old versions)
esbuild-old-go-version:
name: esbuild CI (old Go version)
runs-on: ubuntu-latest

steps:
Expand All @@ -221,3 +219,35 @@ jobs:

- name: make test-old-ts
run: make test-old-ts

esbuild-old-deno-version:
name: esbuild CI (old Deno version)
runs-on: ${{ matrix.os }}
strategy:
matrix:
os: [ubuntu-latest, macos-latest, windows-latest]

steps:
- name: Set up Go 1.x
uses: actions/setup-go@v3
with:
go-version: 1.20.12
id: go

# Make sure esbuild works with old versions of Deno. Note: It's important
# to test a version before 1.31.0, which introduced the "Deno.Command" API.
- name: Setup Deno 1.24.0
uses: denoland/setup-deno@main
with:
deno-version: v1.24.0

- name: Check out code into the Go module directory
uses: actions/checkout@v3

- name: Deno Tests (non-Windows)
if: matrix.os != 'windows-latest'
run: make test-deno

- name: Deno Tests (Windows)
if: matrix.os == 'windows-latest'
run: make test-deno-windows
8 changes: 8 additions & 0 deletions CHANGELOG.md
@@ -1,5 +1,13 @@
# Changelog

## Unreleased

* Work around API deprecations in Deno 1.40.x ([#3609](https://github.com/evanw/esbuild/issues/3609))

Deno 1.40.0 introduced run-time warnings about certain APIs that esbuild uses. With this release, esbuild will work around these run-time warnings by using other APIs if they are present, and falling back to the original APIs otherwise. This should avoid the warnings without breaking compatibility with older versions of Deno.

Unfortunately, doing this introduces a breaking change. The other child process APIs seem to lack a way to synchronously terminate esbuild's child process, which causes Deno to now fail tests with a confusing error about a thing called `op_spawn_wait` in Deno's internals. To work around this, esbuild's `stop()` function has been changed to return a promise, and you now have to change `esbuild.stop()` to `await esbuild.stop()` in all of your Deno tests.
Copy link
Owner Author

Choose a reason for hiding this comment

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

It's unfortunate that this is a breaking change. I think this is significant enough to require a breaking change release for esbuild.

Copy link
Contributor

Choose a reason for hiding this comment

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

which causes Deno to now fail tests with a confusing error about a thing called op_spawn_wait in Deno's internals

Unfortunately there is an error mapping missing in the internals which is why you got this error. It should have said:

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.

Will be fixed in Deno 1.40.2

Copy link
Contributor

Choose a reason for hiding this comment

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

Also the reason we don't have synchronous process termination, is because subprocess termination is by definition not synchronous. When you send a signal with the kill() syscall, the process may intercept it and decide to not exit. If it doesn't exit immediately, you can't just hang the parent process forever.

What the old Deno APIs, and Node's API do to emulate synchronous exit is to just "forget" about the process after close / kill, which means that if the subprocess doesn't actually shut down, you have a zombie process that will get orphaned when the parent process exits. This new API avoids this footgun - it's modeled after the Rust API for subprocesses.


## 0.19.12

* The "preserve" JSX mode now preserves JSX text verbatim ([#3605](https://github.com/evanw/esbuild/issues/3605))
Expand Down
146 changes: 112 additions & 34 deletions lib/deno/mod.ts
Expand Up @@ -43,8 +43,8 @@ export const analyzeMetafileSync: typeof types.analyzeMetafileSync = () => {
throw new Error(`The "analyzeMetafileSync" API does not work in Deno`)
}

export const stop = () => {
if (stopService) stopService()
export const stop = async () => {
if (stopService) await stopService()
}

let initializeWasCalled = false
Expand Down Expand Up @@ -178,63 +178,142 @@ interface Service {

let defaultWD = Deno.cwd()
let longLivedService: Promise<Service> | undefined
let stopService: (() => void) | undefined
let stopService: (() => Promise<void>) | undefined

// Declare a common subprocess API for the two implementations below
type SpawnFn = (cmd: string, options: {
args: string[]
stdin: 'piped' | 'inherit'
stdout: 'piped' | 'inherit'
stderr: 'inherit'
}) => {
stdin: {
write(bytes: Uint8Array): void
close(): void
}
stdout: {
read(): Promise<Uint8Array | null>
close(): void
}
close(): Promise<void> | void
status(): Promise<{ code: number }>
}

// Deno ≥1.40
const spawnNew: SpawnFn = (cmd, { args, stdin, stdout, stderr }) => {
const child = new Deno.Command(cmd, {
args,
cwd: defaultWD,
stdin,
stdout,
stderr,
}).spawn()
const writer = child.stdin.getWriter()
const reader = child.stdout.getReader()
return {
stdin: {
write: bytes => writer.write(bytes),
close: () => writer.close(),
},
stdout: {
read: () => reader.read().then(x => x.value || null),
close: () => reader.cancel(),
},
close: async () => {
// Note: This can throw with EPERM on Windows (happens in GitHub Actions)
child.kill()
Copy link
Owner Author

Choose a reason for hiding this comment

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

Trying to kill the child process on Windows throws PermissionDenied: Access is denied. (os error 5). But not killing the process on Windows causes Deno to fail tests with 1 async operation to op_spawn_wait was started in this test, but never completed. So it seems like maybe there's no way to get tests to pass on Windows with Deno's new APIs? I'm not sure how to proceed here. Maybe this is a bug in Deno itself?

Copy link

@dbushell dbushell Jan 26, 2024

Choose a reason for hiding this comment

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

in stopService you may need to await stdin.close() since writer.close() returns a promise? (same for stdout maybe though Deno docs say only the "stdin WritableStream needs to be closed manually") - sorry I don't have Windows on hand to test.

I think the first test fail because the child process is spawned within fn but closed outside in finally { esbuildNative.stop() }. Seems like Deno test is being overly critical/protective there as the real test is working as intended. I don't know enough to resolve that issue sadly.

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a race between you calling stdin.close() and child.kill(). The esbuild binary probably exits by itself when you close it's stdin to prevent runaway processes. Thus, when you call stdin.close() the process may have already exited by the time you call child.kill() (and then that call fails because the process is already gone).

You can rely on the fact that esbuild shuts down when you close stdin: just make sure you close stdin during shutdown, and then just wait for the process to actually have exited with await child.status.

Copy link
Owner Author

Choose a reason for hiding this comment

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

That seems to have worked. Thanks for the suggestion!


// Without this, Deno will fail tests with some weird error about "op_spawn_wait"
await child.status
},
status: () => child.status,
}
}

// Deno <1.40
const spawnOld: SpawnFn = (cmd, { args, stdin, stdout, stderr }) => {
const child = Deno.run({
cmd: [cmd].concat(args),
cwd: defaultWD,
stdin,
stdout,
stderr,
})
const stdoutBuffer = new Uint8Array(4 * 1024 * 1024)
let writeQueue: Uint8Array[] = []
let isQueueLocked = false

// We need to keep calling "write()" until it actually writes the data
const startWriteFromQueueWorker = () => {
if (isQueueLocked || writeQueue.length === 0) return
isQueueLocked = true
child.stdin!.write(writeQueue[0]).then(bytesWritten => {
isQueueLocked = false
if (bytesWritten === writeQueue[0].length) writeQueue.shift()
else writeQueue[0] = writeQueue[0].subarray(bytesWritten)
startWriteFromQueueWorker()
})
}

return {
stdin: {
write: bytes => {
writeQueue.push(bytes)
startWriteFromQueueWorker()
},
close: () => child.stdin!.close(),
},
stdout: {
read: () => child.stdout!.read(stdoutBuffer).then(n => n === null ? null : stdoutBuffer.subarray(0, n)),
close: () => child.stdout!.close(),
},
close: () => child.close(),
status: () => child.status(),
}
}

let ensureServiceIsRunning = (): Promise<Service> => {
// This is a shim for "Deno.run" for newer versions of Deno
const spawn: SpawnFn = Deno.Command ? spawnNew : spawnOld

const ensureServiceIsRunning = (): Promise<Service> => {
if (!longLivedService) {
longLivedService = (async (): Promise<Service> => {
const binPath = await install()
const isTTY = Deno.isatty(Deno.stderr.rid)
const isTTY = Deno.stderr.isTerminal
? Deno.stderr.isTerminal() // Deno ≥1.40
: Deno.isatty(Deno.stderr.rid) // Deno <1.40

const child = Deno.run({
cmd: [binPath, `--service=${version}`],
cwd: defaultWD,
const child = spawn(binPath, {
args: [`--service=${version}`],
stdin: 'piped',
stdout: 'piped',
stderr: 'inherit',
})

stopService = () => {
stopService = async () => {
// Close all resources related to the subprocess.
child.stdin.close()
child.stdout.close()
child.close()
await child.close()
initializeWasCalled = false
longLivedService = undefined
stopService = undefined
}

let writeQueue: Uint8Array[] = []
let isQueueLocked = false

// We need to keep calling "write()" until it actually writes the data
const startWriteFromQueueWorker = () => {
if (isQueueLocked || writeQueue.length === 0) return
isQueueLocked = true
child.stdin.write(writeQueue[0]).then(bytesWritten => {
isQueueLocked = false
if (bytesWritten === writeQueue[0].length) writeQueue.shift()
else writeQueue[0] = writeQueue[0].subarray(bytesWritten)
startWriteFromQueueWorker()
})
}

const { readFromStdout, afterClose, service } = common.createChannel({
writeToStdin(bytes) {
writeQueue.push(bytes)
startWriteFromQueueWorker()
child.stdin.write(bytes)
},
isSync: false,
hasFS: true,
esbuild: ourselves,
})

const stdoutBuffer = new Uint8Array(4 * 1024 * 1024)
const readMoreStdout = () => child.stdout.read(stdoutBuffer).then(n => {
if (n === null) {
const readMoreStdout = () => child.stdout.read().then(buffer => {
if (buffer === null) {
afterClose(null)
} else {
readFromStdout(stdoutBuffer.subarray(0, n))
readFromStdout(buffer)
readMoreStdout()
}
}).catch(e => {
Expand Down Expand Up @@ -331,9 +410,8 @@ let ensureServiceIsRunning = (): Promise<Service> => {

// If we're called as the main script, forward the CLI to the underlying executable
if (import.meta.main) {
Deno.run({
cmd: [await install()].concat(Deno.args),
cwd: defaultWD,
spawn(await install(), {
args: Deno.args,
stdin: 'inherit',
stdout: 'inherit',
stderr: 'inherit',
Expand Down
1 change: 1 addition & 0 deletions lib/deno/wasm.ts
Expand Up @@ -50,6 +50,7 @@ export const analyzeMetafileSync: typeof types.analyzeMetafileSync = () => {

export const stop = () => {
if (stopService) stopService()
return Promise.resolve()
}

interface Service {
Expand Down
1 change: 1 addition & 0 deletions lib/npm/browser.ts
Expand Up @@ -45,6 +45,7 @@ export const analyzeMetafileSync: typeof types.analyzeMetafileSync = () => {

export const stop = () => {
if (stopService) stopService()
return Promise.resolve()
}

interface Service {
Expand Down
1 change: 1 addition & 0 deletions lib/npm/node.ts
Expand Up @@ -223,6 +223,7 @@ export let analyzeMetafileSync: typeof types.analyzeMetafileSync = (metafile, op
export const stop = () => {
if (stopService) stopService()
if (workerThreadService) workerThreadService.stop()
return Promise.resolve()
}

let initializeWasCalled = false
Expand Down
2 changes: 1 addition & 1 deletion lib/shared/types.ts
Expand Up @@ -673,4 +673,4 @@ export let version: string
// 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Also FYI, the new API has Deno.ChildProcess#ref and Deno.ChildProcess#unref methods that should let most users avoid calling this API (unless they are running in a test with sanitization, like you are here).

I suggest you call process.unref() like you do in Node, and then most users don't have to call this method at all anymore :)

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes, that comment is old. Thanks for mentioning it. I just became aware of unref while doing this yesterday and still need to look into it.

Although thinking about the bigger picture, I suppose perhaps this whole deno.land/x/esbuild library is unnecessary now that Deno has since added support for npm packages. It feels like I should just deprecate and abandon it, and then tell people to import from npm:esbuild instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is certainly an option - however in that case a user may still want to support the explicit shutdown of the underlying esbuild process.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes, that's true. Calling esbuild.stop() from npm:esbuild currently prints the following:

Warning: Not implemented: ChildProcess.prototype.disconnect

I'm not sure why though as esbuild never calls disconnect and there is no stack trace associated with the warning.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Note: Calling unref seems to have caused a regression: #3682. I'm removing the call to unref to fix the regression: #3685.

export declare function stop(): void;
export declare function stop(): Promise<void>
6 changes: 3 additions & 3 deletions scripts/deno-tests.js
Expand Up @@ -37,7 +37,7 @@ function test(name, backends, fn) {
await fn({ esbuild: esbuildNative, testDir })
await Deno.remove(testDir, { recursive: true }).catch(() => null)
} finally {
esbuildNative.stop()
await esbuildNative.stop()
}
})
break
Expand All @@ -51,7 +51,7 @@ function test(name, backends, fn) {
await fn({ esbuild: esbuildWASM, testDir })
await Deno.remove(testDir, { recursive: true }).catch(() => null)
} finally {
esbuildWASM.stop()
await esbuildWASM.stop()
}
})
break
Expand All @@ -65,7 +65,7 @@ function test(name, backends, fn) {
await fn({ esbuild: esbuildWASM, testDir })
await Deno.remove(testDir, { recursive: true }).catch(() => null)
} finally {
esbuildWASM.stop()
await esbuildWASM.stop()
}
})
break
Expand Down
7 changes: 4 additions & 3 deletions scripts/esbuild.js
Expand Up @@ -8,6 +8,7 @@ const denoDir = path.join(repoDir, 'deno')
const npmDir = path.join(repoDir, 'npm', 'esbuild')
const version = fs.readFileSync(path.join(repoDir, 'version.txt'), 'utf8').trim()
const nodeTarget = 'node10'; // See: https://nodejs.org/en/about/releases/
const denoTarget = 'deno1'; // See: https://nodejs.org/en/about/releases/
const umdBrowserTarget = 'es2015'; // Transpiles "async"
const esmBrowserTarget = 'es2017'; // Preserves "async"

Expand Down Expand Up @@ -227,7 +228,7 @@ const buildDenoLib = async (esbuildPath) => {
path.join(repoDir, 'lib', 'deno', 'mod.ts'),
'--bundle',
'--outfile=' + path.join(denoDir, 'mod.js'),
'--target=esnext',
'--target=' + denoTarget,
'--define:ESBUILD_VERSION=' + JSON.stringify(version),
'--platform=neutral',
'--log-level=warning',
Expand All @@ -237,11 +238,11 @@ const buildDenoLib = async (esbuildPath) => {
// Generate "deno/wasm.js"
const GOROOT = childProcess.execFileSync('go', ['env', 'GOROOT']).toString().trim()
let wasm_exec_js = fs.readFileSync(path.join(GOROOT, 'misc', 'wasm', 'wasm_exec.js'), 'utf8')
const wasmWorkerCode = await generateWorkerCode({ esbuildPath, wasm_exec_js, minify: true, target: 'esnext' })
const wasmWorkerCode = await generateWorkerCode({ esbuildPath, wasm_exec_js, minify: true, target: denoTarget })
const modWASM = childProcess.execFileSync(esbuildPath, [
path.join(repoDir, 'lib', 'deno', 'wasm.ts'),
'--bundle',
'--target=esnext',
'--target=' + denoTarget,
'--define:ESBUILD_VERSION=' + JSON.stringify(version),
'--define:WEB_WORKER_SOURCE_CODE=' + JSON.stringify(wasmWorkerCode),
'--platform=neutral',
Expand Down