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

feat: easier way to destroy child processes #20497

Open
marvinhagemeister opened this issue Sep 14, 2023 · 2 comments
Open

feat: easier way to destroy child processes #20497

marvinhagemeister opened this issue Sep 14, 2023 · 2 comments
Labels
public API related to "Deno" namespace in JS runtime Relates to code in the runtime crate suggestion suggestions for new features (yet to be agreed)

Comments

@marvinhagemeister
Copy link
Contributor

After a debugging session regarding leaking resources in tests it turned out that a test case in the Fresh repository wasn't properly waiting for child processes to be killed.

The current way to do it should look like this:

const childProcess = new Deno.Command(...).spawn();

// Send kill signal
childProcess.kill("SIGTERM");
// Wait until it's shut down finished. This is often forgotten
await childProcess.status;

This lead us to wonder if we could provide an easier API for that. Especially, in a test suite it's not relevant how the child process is killed. You just want this thing to be destroyed. Some random ideas:

// New method
await childProcess.killAndWait();

// Make kill() return Promise, doesn't work with every signal though so meh
await childProcess.kill("SIGTERM")

// Add necessary symbols to support "using"
using childProcess = new Deno.Command(...).spawn();
@lucacasonato
Copy link
Member

It's to be noted that with #20501 the accuracy of the op sanitizer has improved to the point where forgetting await childProcess.status will fail the test with a sanitizer error, if the sanitizers are enabled (as they are by default).

We should still ponder this, but it's less urgent now.

@lucacasonato lucacasonato added public API related to "Deno" namespace in JS suggestion suggestions for new features (yet to be agreed) runtime Relates to code in the runtime crate labels Sep 18, 2023
@magurotuna
Copy link
Member

Now that delcaration with using is supported and ChildProcess implements AsyncDisposable, we can easily ensure child processes are destructed once they get out of scope:

await using proc = new Deno.Command(...).spawn();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
public API related to "Deno" namespace in JS runtime Relates to code in the runtime crate suggestion suggestions for new features (yet to be agreed)
Projects
None yet
Development

No branches or pull requests

3 participants