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

Conversation

evanw
Copy link
Owner

@evanw evanw commented Jan 26, 2024

Deno 1.40.0 introduced run-time warnings about certain APIs that esbuild uses. With this PR, esbuild will work around these run-time warnings by using newer 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 newer child process APIs lack a way to synchronously terminate esbuild's child process, so calling esbuild.stop() from within a Deno test is no longer sufficient to prevent Deno from failing a test that uses esbuild's API (Deno fails tests that create a child process without killing it before the test ends). 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.

Fixes #3609

CHANGELOG.md Outdated

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.

lib/deno/mod.ts Outdated
},
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!

Comment on lines 673 to 675
// 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deno: Usage of deprecated API with >= Deno 1.40
3 participants