Skip to content

fix(cli): ctrl+C no longer kills processes (#11434)#11518

Merged
patak-cat merged 3 commits intovitejs:mainfrom
kinfuy:fix/ctrl-c-bug
Jan 2, 2023
Merged

fix(cli): ctrl+C no longer kills processes (#11434)#11518
patak-cat merged 3 commits intovitejs:mainfrom
kinfuy:fix/ctrl-c-bug

Conversation

@kinfuy
Copy link
Copy Markdown
Contributor

@kinfuy kinfuy commented Dec 29, 2022 •
edited
Loading

Description

The binding of shortcut keys makes Ctrl+C unable to exit the program directly. Although the SIGTERM signal is sent upward, it cannot end when the console has an asynchronous output task. I think Ctrl+C should exit process.exit(1)

Additional context

close #11434

What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the PR Title Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@kinfuy kinfuy changed the title fix: Ctrl+C no longer kills processes (#11434) fix(cli): Ctrl+C no longer kills processes (#11434) Dec 29, 2022
@ArnaudBarre
Copy link
Copy Markdown
Member

ArnaudBarre commented Dec 29, 2022

I don't remember what but it causes issues. I need to look at it a bit closer

@sapphi-red
Copy link
Copy Markdown
Member

sapphi-red commented Dec 31, 2022

I guess it's because await server.close() won't be called if process.exit() is used.

@kinfuy kinfuy mentioned this pull request Jan 1, 2023
Merged
9 tasks
@ArnaudBarre ArnaudBarre mentioned this pull request Jan 1, 2023
Closed
ArnaudBarre
ArnaudBarre reviewed Jan 1, 2023
View reviewed changes
@ArnaudBarre ArnaudBarre changed the title fix(cli): Ctrl+C no longer kills processes (#11434) fix(cli): ctrl+C no longer kills processes (#11434) Jan 2, 2023
bluwy
bluwy approved these changes Jan 2, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Really neat solution 👍

@patak-cat patak-cat merged commit 718fc1d into vitejs:main Jan 2, 2023
@patak-cat patak-cat added the p3-minor-bug An edge case that only affects very specific usage (priority) label Jan 2, 2023
ArnaudBarre added a commit that referenced this pull request Jan 3, 2023
@ArnaudBarre
revert: fix(cli): ctrl+C no longer kills processes (#11434) (#11518)
4fcbb0e
This was referenced Jan 3, 2023
Merged
Merged
patak-cat pushed a commit that referenced this pull request Jan 3, 2023
@ArnaudBarre
fix(cli): revert ctrl+C no longer kills processes (#11434) (#11518) (#…
3748acb 
…11562)
@kinfuy kinfuy mentioned this pull request Jan 10, 2023
Closed
@liangxiwei
Copy link
Copy Markdown

liangxiwei commented Feb 6, 2023

stil not working in 4.1.1

@liangxiwei
Copy link
Copy Markdown

liangxiwei commented Feb 10, 2023

@kinfuy kinfuy

@ArnaudBarre
Copy link
Copy Markdown
Member

ArnaudBarre commented Feb 12, 2023

Can you provide a repro?
Just tested with npm-run-all & vite 4.1.1, it works (both yarn & pnpm)

@lincenying
Copy link
Copy Markdown

lincenying commented Feb 20, 2023 •
edited
Loading

After version 4.1.0, press ctrl+c to stop the service, and the console will output:

error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

image

@kinfuy
Copy link
Copy Markdown
Contributor Author

kinfuy commented Feb 20, 2023

The current processing is that ctrl c directly throws process.exit (1), which is indeed consistent with the behavior of ctrl c's unexpected exit, but this prompt will make people feel sick. Maybe there is a need to find a better way

@ArnaudBarre
Copy link
Copy Markdown
Member

ArnaudBarre commented Feb 20, 2023

Yeah I was annoyed by this extra log but with the team we decided to go in that direction so everything is supported (shortcuts, running via npm-run-all) without configuration.

The only other solution would be to allow people to out-out of shortcuts bindings. But that's one more thing to document.
For now you can use the JS API is really you don't want this behaviour

futurGH pushed a commit to futurGH/vite that referenced this pull request Feb 26, 2023
@kinfuy @ArnaudBarre @futurGH
fix(cli): ctrl+C no longer kills processes (vitejs#11434) (vitejs#11518)
67e2bb3 
Co-authored-by: Arnaud Barré <[email protected]>
close vitejs#11434
futurGH pushed a commit to futurGH/vite that referenced this pull request Feb 26, 2023
@ArnaudBarre @futurGH
fix(cli): revert ctrl+C no longer kills processes (vitejs#11434) (vit…
856f8f1 
…ejs#11518) (vitejs#11562)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ArnaudBarre ArnaudBarre ArnaudBarre approved these changes

@bluwy bluwy bluwy approved these changes

Assignees

No one assigned

Labels

p3-minor-bug An edge case that only affects very specific usage (priority)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Ctrl+C no longer kills processes running in parallel with vite

7 participants

@kinfuy @ArnaudBarre @sapphi-red @liangxiwei @lincenying @bluwy @patak-cat