feat: introduce better way for handling Ctrl+C #4533
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When Ctrl+C is used in terminal, CLI receives SIGINT signal and tries to clear its resources. In Node.js, when you handle SIGINT signal, you'll have to clean all resources on your own. This leads to different problems, for example in case you press Ctrl+C during doctor execution, CLI will only kill the doctor process, but will continue working. Instead of handling the SIGINT signal, let the OS handle it on its own. This works quite well except one thing - any package that is required may add handler for SIGINT signal and this will break the idea. So repace the `process.on` function with our own one which does not allow adding handler for SIGINT signal.
With recent changes, CLI's processService does not handle SIGINT signal, so it actually never sends Finish message to analytics process. CLI dies gracefully, but the analytics process is not killed. To fix this add `process.exit` in the analytics process when it receives disconnect event. Also remove all logic for Finish message as CLI will never send it.
In case CLI uses non-Nodejs resources, there's nowhere to clean them as CLI does not handle the SIGINT (Ctrl+C) signal. For example, when we debug on Android, we setup port forward with `adb` command. Once CLI process is about to stop, the adb forward should be removed, but currently there's nowhere to do it. So, to handle this, introduce a new cleanup child process. It is detached and unrefed. As we have IPC communication with it, when CLI exits gracefully (i.e. when we are not in a long living command which usually exits with Ctrl+C), we should manually disconnect the process. Whenever some external resources must be cleaned, the actual cleanup commands that should be executed are sent to the new process. It caches them and once the CLI finishes its work, the cleanup process will receive `disconnect` event. At this point the cleanup process will execute all cleanup actions and after that it will exit. The idea of the process is very similar to the process used for analytics tracking, so several interfaces were reused. They are just renamed to have a better meaning for the new used cases. Introduce `--cleanupLogFile` option, which allows the user to specify path to file, where information about all actions will be logged. Also expose a method in CLI's public API that allows setting this file when using CLI as a library.
Currently it is possible just to add new actions to be executed in the cleanup. However, in some cases, we want to add action and remove it after that. Also, we need to delete some lock files when CLI is not using them anymore. So, introduce new API in the cleanup service that allow all of these actions.
Replace processService with cleanupService where appropriate. The cleanupService allows execution of code after CLI has died. `processService` was used to handle some events, but it caused more issues than benefits. So remove it from the codebase.
Remove previously introduced `$timers` - the idea was to use $timers as we wanted to kill remaining timers when SIGINT is sent to CLI. As CLI no longer handles SIGINT signal, Node.js kills the timers on its own, so there's no need of additional timers implementation.
Add information in PublicAPI for cleanupService and its methods.
In Node.js you can set timeout option to child processes started with `exec`. However, there's no support for this when using `spawn`. Add support for passing timeout in our abstraction of childProcess, when using `spawnFromEvent` method. When this method is used, it blocks the operation until the process finishes its execution. So it is appropriate to pass timeout and if it passes, send `SIGTERM` signal to the child process. This is the same signal used by Node.js when the timeout is hit in `child_process.exec`.
Add default timeout for each cleanup operation. Set it to 3000ms by default. For each action, you can set the timeout when adding it to the cleanup action list by passing timeout.
ghost
added
the
|
test cli-smoke cli-preview |
DimitarTachev
approved these changes
Apr 12, 2019
`lockService` creates a file to be locked and next to it - directory with the same name and `.lock` extension. Both should be cleaned when CLI process is dies, so handle both of them.
Improve interfaces and enums for cleanup services.
Comment the code that removes the port forwarding for Android as this is a breaking change in the behavior. Uncomment the code for 6.0.0 release.
|
test cli-smoke cli-preview |
Currently the cleanup-process is never disconnected from CLI as in the `commandsService` we set the `shouldDispose` property to false. This breaks short living commands like `tns create`, `tns devices`, etc. Handle the disposing similar to the other long living commands in CLI.
|
test cli-smoke cli-preview cli-misc |
|
Merge after telerik/nativescript-cloud#167 |
|
test cli-preview |
|
test cli-templates |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
During its work, CLI uses different resources that should be cleaned once CLI process finishes its work. There are several ways for CLI to exit:
tns version,tns run <platform> --justlaunchare executed and at the end of the execution,$injector.disposeis called. At this point thedisposemethods of each of the injected dependencies is called. That's where you can clean the used resources. The main purpose of thedisposeevent is to cleanup some resources like timers, which will prevent CLI process from exit gracefully.tns run,tns debug <platform>are usually killed withCtrl + C(SIGINT signal is sent to CLI). To handle this we've added handling for SIGINT signal in CLI.However, handling SIGINT in CLI led to a problem. As per Node.js documentation:
So after we've added the handling, we had to manually cleanup all resources that prevent Node.js from exiting gracefully, i.e. pending Promises, pending timers, child processes, etc. These operations take time and users started complaining that pressing Ctrl + C does not exit the process and they have to press it several times. Also many users started pressing Ctrl + Z, which just sends CLI process in background, but they thought this kills the process as the terminal is not blocked by CLI anymore.
So we needed a solution how to cleanup some resources that Node.js does not clean when exiting on its own, but also we did not want to take care of killing all child_processes and timers.
By default, when Node.js receives SIGINT it kills all child processes started from the current process, clears the pending timers and exits. Detached processes are not killed as by default they are not related to current process.
So this PR adds a new detached child process that executes the cleanup procedures once CLI process exits. The child process is detached, so Node.js will not kill it when Ctrl + C is used to stop CLI's process. The child process communicates with CLI via IPC communication. Once CLI process dies, the child process will receive
disconnectevent and will start execution of cleanup tasks.You can add/remove cleanup actions by using the newly introduced
$cleanupServicein the CLI. With it you can add cleanup actions which are actually processes that should be spawned from the cleanup service, for exampleadbcommand with some args. For each action you can specify command, args and timeout. In case the cleanup process is unable to execute the cleanup action for specified time (default is 3 seconds), the current cleanup action is stopped and next one starts.As for our
lockfilewe need to ensure some locks are deleted once CLI dies, the$cleanupServiceallows you to specify file/directory that should be deleted from the cleanup process.As the cleanup process is detached, we do not have any logs from it. This makes it hard to understand if it actually does its job, so add a way to have logs from this file. In CLI this happens by passing
--cleanupLogFile <path to file>. As CLI can be used as a library, introduce public method that allows setting the logs file location.The whole effort made current implementation of handling exit signals (
$processService) unusable, so it is deleted and where it is appropriate it is replaced by$cleanupService.More information for the changes is available in each commit.
PR Checklist
What is the current behavior?
Running some long living CLI commands, like
tns run,tns debug <platform>, etc. and trying to stop them withCtrl + Cdoes not stop the process immediately or causes different unexpected logs. In some cases the process does not die at all.What is the new behavior?
Pressing
Ctrl + Ckills CLI process immediately.Fixes issue: #3993