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

Feature/430 fix unclean shutdown #580

Merged
merged 1 commit into from
May 17, 2017

Conversation

phoerious
Copy link
Member

@phoerious phoerious commented May 17, 2017

Description

This is a partial fix for issue #430. It resolves shutdown problems on OS X when using the "Quit" action from the right-click menu on the dock.

It also contains an incomplete clean shutdown implementation for Windows. The signaling mechanism via CreateEvent() and SetEvent() works in general, but trapping the actual system events doesn't. The control handler registered by SetConsoleCtrlHandler() isn't called. Maybe someone more knowledgeable in Windows API internals can help me here. I suppose it may be that Windows doesn't treat KeePassXC as a console application or some other stupid reason.

EDIT: Windows implementation canceled, considered a non-issue (mostly).

How has this been tested?

Ran on macOS Sierra and Windows 10.

Types of changes

  • ✅ Bug fix (non-breaking change which fixes an issue)

Checklist:

  • ✅ I have read the CONTRIBUTING document. [REQUIRED]
  • ✅ My code follows the code style of this project. [REQUIRED]
  • ✅ All new and existing tests passed. [REQUIRED]
  • ✅ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]

@TheZ3ro
Copy link
Contributor

TheZ3ro commented May 17, 2017

@phoerious Is this a WIP on Windows side or it's ready to merge?

@phoerious
Copy link
Member Author

I don't know. If someone has any ideas why it's not working, I'd appreciate any hints.

@phoerious phoerious force-pushed the feature/430-fix-unclean-shutdown branch from 85deb78 to b68c7a1 Compare May 17, 2017 20:25
@phoerious
Copy link
Member Author

phoerious commented May 17, 2017

I did some more research, but couldn't find anything else that would help me.

In addition to what I already submitted, I also tried to make it work using signal() from signal.h, but without success. It appears that Ctrl+C just cannot be trapped from an Msys terminal. Neither approach allowed me to trap interrupt signals/events.

Since the task manager seems to send a clean close signal to the window anyway and not an interrupt signal when clicking "End Task", I decided to undo my last commit on this patch and only submit the macOS code. The behavior upon Windows shutdown should be about the same as the behavior of the task manager.

I consider #430 fixed with this. It works well on Linux and macOS and on Windows it cannot really be fixed, but also doesn't appear to be much of an issue in the first place.

@TheZ3ro
Copy link
Contributor

TheZ3ro commented May 17, 2017

Is this related keepassx/keepassx#169 ?

@phoerious
Copy link
Member Author

No, that's a different problem.

@TheZ3ro
Copy link
Contributor

TheZ3ro commented May 17, 2017

Ok so I will test and review this 😉

PS: Can you first review and merge #545 and then rebase this?

@phoerious
Copy link
Member Author

I tested it already and only marked a few code style problems. Once those are fixed, it can be merged.

@phoerious phoerious force-pushed the feature/430-fix-unclean-shutdown branch from b68c7a1 to 00dc4b9 Compare May 17, 2017 20:44
Copy link
Contributor

@TheZ3ro TheZ3ro left a comment

Choose a reason for hiding this comment

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

Looks good

@phoerious phoerious merged commit aa64b2e into develop May 17, 2017
@phoerious phoerious deleted the feature/430-fix-unclean-shutdown branch May 17, 2017 21:18
@Keisial
Copy link

Keisial commented May 17, 2017

signal(SIGINT, ...) should work fine on Windows (at least it used to). Are you building it as a console or Windows application? In the later case, it would not have a console attached (but I don't see how you could be sending Ctrl-C from msys either).

@phoerious
Copy link
Member Author

I don't think I build it as a console app. When I start it from the Windows command line, it immediately detaches from the console, but when I start it from Msys2, it stays connected to the shell, so that I can exit it with Ctrl+C. When I print stuff to. STDOUT, I also see it in the terminal, but Ctrl+C just quits immediately as if it sent a SIGKILL.

@Keisial
Copy link

Keisial commented May 19, 2017

msys may be doing something odd here in order to "reattach" the program. I would try rebuilding with the -mwindows flag removed from the linking step.
Also, what's the behavior when you kill -SIGINT from a different console?

droidmonkey added a commit that referenced this pull request Jun 25, 2017
- Added YubiKey 2FA integration for unlocking databases [#127]
- Added TOTP support [#519]
- Added CSV import tool [#146, #490]
- Added KeePassXC CLI tool [#254]
- Added diceware password generator [#373]
- Added support for entry references [#370, #378]
- Added support for Twofish encryption [#167]
- Enabled DEP and ASLR for in-memory protection [#371]
- Enabled single instance mode [#510]
- Enabled portable mode [#645]
- Enabled database lock on screensaver and session lock [#545]
- Redesigned welcome screen with common features and recent databases [#292]
- Multiple updates to search behavior [#168, #213, #374, #471, #603, #654]
- Added auto-type fields {CLEARFIELD}, {SPACE}, {{}, {}} [#267, #427, #480]
- Fixed auto-type errors on Linux [#550]
- Prompt user prior to executing a cmd:// URL [#235]
- Entry attributes can be protected (hidden) [#220]
- Added extended ascii to password generator [#538]
- Added new database icon to toolbar [#289]
- Added context menu entry to empty recycle bin in databases [#520]
- Added "apply" button to entry and group edit windows [#624]
- Added macOS tray icon and enabled minimize on close [#583]
- Fixed issues with unclean shutdowns [#170, #580]
- Changed keyboard shortcut to create new database to CTRL+SHIFT+N [#515]
- Compare window title to entry URLs [#556]
- Implemented inline error messages [#162]
- Ignore group expansion and other minor changes when making database "dirty" [#464]
- Updated license and copyright information on souce files [#632]
- Added contributors list to about dialog [#629]
@phoerious phoerious added pr: bugfix Pull request that fixes a bug and removed bug labels Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform: macOS pr: bugfix Pull request that fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants