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

CLI: Gracefully shutdown and cleanup execa child processes #23538

Merged
merged 8 commits into from
Jul 21, 2023

Conversation

valentinpalkovic
Copy link
Contributor

Closes N/A

What I did

Execa child processes should be cleanup up as soon as the parent process quits. During initialization, the "cancel" event should properly be sent, and it should not lead to error events sent.

Also set the cleanup option to all our execa command executions, since I think the child processes shouldn't continue to run if the parent process exits.

How to test

  1. Run the init event like STORYBOOK_TELEMETRY_DEBUG=1 <storybook-folder>/code/lib/cli/bin/index.js init --force in one of the sandboxes.
  2. Exit at any time. The cancel event should be sent without any following error event or any other logs from e.g. yarn
  3. The events should be logged to the console in the following order:
  • boot(init)
  • init
  • boot(dev)
  • dev
  • nothing if I ctl-C

Checklist

  • Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
  • Make sure to add/update documentation regarding your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli/src/sandbox-templates.ts
  • Make sure this PR contains one of the labels below.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "build", "documentation", "maintenance", "dependencies", "other"]

Execa child processes should be cleanup up as soon as the parent process quits. During initialization the "cancel" event should properly be sent and it should not lead to error events sent.
@valentinpalkovic valentinpalkovic self-assigned this Jul 20, 2023
@valentinpalkovic valentinpalkovic added patch:yes Bugfix & documentation PR that need to be picked to main branch ci:daily Run the CI jobs that normally run in the daily job. labels Jul 20, 2023
@valentinpalkovic valentinpalkovic changed the title Gracefully shutdown and cleanup execa child processes CLI: Gracefully shutdown and cleanup execa child processes Jul 20, 2023
@valentinpalkovic valentinpalkovic marked this pull request as ready for review July 20, 2023 11:59
@yannbf
Copy link
Member

yannbf commented Jul 20, 2023

I tested this flow in a react-vite sandbox:

  1. Run STORYBOOK_TELEMETRY_DEBUG=1 <storybook-folder>/code/lib/cli/bin/index.js init --force
  2. Wait until Storybook successfully runs after init
  3. Ctrl C

The events were as following:

boot(init)
init
boot(dev)
dev
canceled(init)
error(init)

@yannbf yannbf merged commit 779b4f6 into next Jul 21, 2023
@yannbf yannbf deleted the valentin/proper-error-handling-and-logging branch July 21, 2023 15:13
JReinhold pushed a commit that referenced this pull request Jul 24, 2023
…ling-and-logging

CLI: Gracefully shutdown and cleanup execa child processes
(cherry picked from commit 779b4f6)
@github-actions github-actions bot added the patch:done Patch/release PRs already cherry-picked to main/release branch label Jul 24, 2023
@github-actions github-actions bot mentioned this pull request Jul 26, 2023
30 tasks
@valentinpalkovic
Copy link
Contributor Author

@mrharispe Could be. Could you open a new issue and provide a reproduction, if possible?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug ci:daily Run the CI jobs that normally run in the daily job. patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch telemetry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants