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: Gracefully exit on Signals #327

Closed
wants to merge 6 commits into from
Closed

feat: Gracefully exit on Signals #327

wants to merge 6 commits into from

Conversation

coder3101
Copy link
Member

@coder3101 coder3101 commented May 7, 2020

Description

Adds Signal handler with help of QCtrlControl

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate)

Type of changes

  • Bug fix (changes which fix an issue)
  • New feature (changes which add functionality)
  • Documentation (changes which modify the documentation only)
  • Style change (changes which do not affect the meaning of the code: code formatting, etc.)
  • Refactor (changes which affect the meaning of the code but neither fix a bug nor add a feature)
  • Performance improve (changes which improve performance)
  • Test (changes which add tests)
  • Build (Changes that affect the build system or external dependencies)
  • CI (changes to CI configuration files and scripts)
  • Chore (changes which do not belong to any type above)
  • Revert (revert previous changes)

Checklist

  • I have read the CONTRIBUTING document.
  • I have tested these changes locally, and this fixes the bug/the new feature behaves as the expectation.
  • I have used clang-format-9 and .clang-format file in the root directory to format my codes.
  • The settings file in the old version can be used in the new version after this change.
  • These changes only fix a single bug/introduces a single feature. (Otherwise, open multiple Pull Requests instead, unless these bugs/features are closely related.)
  • The commit messages are clear and detailed. (Otherwise, use git reset and commit again, or use git rebase -i and git commit --amend to modify the commit messages.)
  • These changes don't remove an existing feature. (Otherwise, add an option to disable this feature instead, unless it's necessary to remove this feature.)
  • I have documented these changes in CHANGELOG.md, or these changes are not notable.

@pull-assistant
Copy link

pull-assistant bot commented May 7, 2020

Score: 0.97

Best reviewed: commit by commit


Optimal code review plan (1 warning)

     Add QCtrlSignals as dependency for Signal Handling

Add initial support for signal handling

...c/Settings/SettingsManager.cpp 50% changes removed in style: Format codes ...

src/appwindow.hpp 60% changes removed in style: Format codes ...

     style: Format codes (#326)

     feat: Update handling signals

     feat: always force-close on Windows

     Use settings.json from handle-signal branch

Powered by Pull Assistant. Last update e454f99 ... 0d0aa9a. Read the comment docs.

@coder3101 coder3101 linked an issue May 7, 2020 that may be closed by this pull request
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@coder3101
Copy link
Member Author

I have left the handing signal with a comment on what should be expected behaviour or what could be better than this as per your wish. I have ported the changes to appwindow and other helper API from #268 into this.

I also confirm that now, I am able to exit gracefully (with hot exit enabled) and that this library works fine on my windows.

@coder3101 coder3101 added the enhancement New feature or request label May 7, 2020
@coder3101 coder3101 mentioned this pull request May 7, 2020
9 tasks
@coder3101 coder3101 changed the title Signal handle feat: Gracefully exit on Signals May 7, 2020
@coder3101 coder3101 requested a review from ouuan May 7, 2020 08:13
@coder3101 coder3101 added the work in progress The work has been started and is in progress. label May 7, 2020
@ouuan
Copy link
Member

ouuan commented May 7, 2020

Now it doesn't catch SIGHUP, which is sent on Linux when closing the terminal where the application was started 🤔. However, I saw SIGHUP in qctrlsignalhandler_unix.cpp, we may need to find out how to catch it later.

And, without this:

emit requestToExit();

, the application won't be killed after receiving SIGTERM during the progress dialog.

@ouuan
Copy link
Member

ouuan commented May 7, 2020

Please test the latest commit first.

@ouuan ouuan marked this pull request as draft May 7, 2020 17:04
@coder3101
Copy link
Member Author

coder3101 commented May 7, 2020

Please test the latest commit first.

It still works on my Windows but when Hotexit is not enabled SIGINT opens the dialog box to ask for saving changes, and same with killing from task manager.

If it will pop open the closeConfirm dialog what good it is? I thought application would exit normally and in next launch show a dialog "Unexpected close, want to restart?"

I have not still ran this commit, will confirm this behaviour in 5-6 hours, I tested locally by putting exact code as you have for signal handling now, (copy pasted old handling code) and found this behaviour during development of this PR

@ouuan
Copy link
Member

ouuan commented May 8, 2020

If it will pop open the closeConfirm dialog what good it is? I thought application would exit normally and in next launch show a dialog "Unexpected close, want to restart?"

I think this is useful on Linux. SIGINT is the same as a normal close, so the user can use Ctrl+C to normally close it. It is strange that the dialog is also showed when it's killed from the Windows task manager, maybe we can treat SIGINT as SIGTERM on Windows.

@ouuan
Copy link
Member

ouuan commented May 11, 2020

I just noticed that d5ea61b has been not working since the new preferences window, because now the settings are saved when normally exiting. After this PR is finished and merged, the feature introduced by that commit will be more useless, so instead of making it work again, we can simply remove it.

@coder3101
Copy link
Member Author

What behaviour you want when SIGTERM and SIGINT is invoked in Windows( from git bash)? I will try to fix it tell me behaviour with Hotexit and without Hotexit?

Note that killing from Task manager sends SIGINT

You work on behaviour for UNIX.

ouuan and others added 2 commits May 13, 2020 18:01
Because SIGINT is sent when killing from the task manager on Windows,
it's better to force-close when receiving SIGINT on Windows.
@coder3101 coder3101 closed this May 16, 2020
@coder3101 coder3101 deleted the signal-handle branch May 16, 2020 06:13
coder3101 added a commit that referenced this pull request May 16, 2020
In this PR #327, started freeing of stack memory and caused runtime
error during destruction.
ouuan pushed a commit that referenced this pull request May 16, 2020
* fix(EditorThemes): Invalid free/delete

In this PR #327, started freeing of stack memory and caused runtime
error during destruction.

* style: Format codes (#355)

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request work in progress The work has been started and is in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Catch system signals like SIGINT
2 participants