-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Changes from 3 commits
3b499bf
5e4663f
31393ac
b4cfc12
665c5fc
9469a31
11ec95f
dd139a7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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(): Promise<void> | void | ||
} | ||
stdout: { | ||
read(): Promise<Uint8Array | null> | ||
close(): Promise<void> | 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() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Trying to kill the child process on Windows throws There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in I think the first test fail because the child process is spawned within There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks like a race between you calling You can rely on the fact that esbuild shuts down when you close There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.stdin.close() | ||
await child.stdout.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 => { | ||
|
@@ -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', | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also FYI, the new API has I suggest you call There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Although thinking about the bigger picture, I suppose perhaps this whole There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that's true. Calling
I'm not sure why though as esbuild never calls There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
export declare function stop(): void; | ||
export declare function stop(): Promise<void> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately there is an error mapping missing in the internals which is why you got this error. It should have said:
Will be fixed in Deno 1.40.2
There was a problem hiding this comment.
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.