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

main: properly handle SIGINT #75999

Merged
merged 4 commits into from
Oct 21, 2024
Merged

main: properly handle SIGINT #75999

merged 4 commits into from
Oct 21, 2024

Conversation

andrei8l
Copy link
Contributor

@andrei8l andrei8l commented Aug 27, 2024

Summary

None

Purpose of change

  • We're handling SIGINT (Ctr+C) by killing ourselves
  • Killing ourselves doesn't immediately terminate execution
  • Sometimes the OS doesn't terminate us in time and execution proceeds as if SIGINT was cancelled and the game tries to refresh display, leading to a segfault since we've already manually destroyed some game objects.
  • Additionally, the sig handler does not comply with MSC54-CPP, so we've been relying on UB this entire time

Describe the solution

Split SIGINT handling to a separate function
Move all non-SIG30-C-compliant code out of the sig handler
Add a new quit reason (QUIT_EXIT) and pass it up to the main loop as an exception, then exit gracefully.

Describe alternatives you've considered

Give up on pedantry and just revert #67893 since the old code worked fine and for all edge cases despite being wrong.

Instead of passing an exception, I could pepper QUIT_EXIT checks all over the place.

Testing

Send SIGINT or other signals, or try to close the window while playing the game in various states: opening menu, loading files, finalizing data, verifying data, loading save file, waiting for input, doing an activity, and in various UIs. The game should prompt and quit correctly with no crashes.

Additional context

It can take a long time for the prompt to appear during some init stages. More QUIT_EXIT checks can be peppered around the code if needed.

UIs that don't use a QUIT keybinding won't react to SIGINT.

There might be other edge cases. Doing it correctly without an unified input loop is hard work.

@github-actions github-actions bot added [C++] Changes (can be) made in C++. Previously named `Code` new contributor <Bugfix> This is a fix for a bug (or closes open issue) astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions labels Aug 27, 2024
@andrei8l andrei8l changed the title main: properly handle SINGINT main: properly handle SIGINT Aug 27, 2024
@andrei8l andrei8l marked this pull request as draft August 27, 2024 18:25
@andrei8l

This comment was marked as outdated.

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Aug 27, 2024
@alef
Copy link
Contributor

alef commented Aug 27, 2024

Could close the 10 years old #8417 too ?

@NetSysFire
Copy link
Member

Does this fix the root cause of the heap overflow, causing corrupted crash path locations?

@andrei8l
Copy link
Contributor Author

Could close the 10 years old #8417 too ?

It does now, thanks.

Does this fix the root cause of the heap overflow, causing corrupted crash path locations?

This PR turned out to fix an unrelated crash.

@andrei8l andrei8l marked this pull request as ready for review August 27, 2024 20:23
@andrei8l andrei8l marked this pull request as draft August 27, 2024 20:57
@NetSysFire
Copy link
Member

to be honest I dont think my PR should be invalidated until theres a proper fix for the mystery heap overflow. I already did everything I can to debug this.

@andrei8l
Copy link
Contributor Author

to be honest I dont think my PR should be invalidated until theres a proper fix for the mystery heap overflow. I already did everything I can to debug this.

There is no mystery. The crash log shows the problem fairly clearly. There are just multiple, unrelated instances of the same issue. In any case, your PR just hides the issue under the rug as I've tried to explain on discord as well.

@NetSysFire
Copy link
Member

So the other PR does fix the underlying issue leading to the heap overflow?

@harakka
Copy link
Member

harakka commented Oct 20, 2024

It seems like the quit dialog gets stuck if you send another sigint while it's open.

Otherwise from some quick testing this seems to work as expected 👍

NB: only tested this on tiles

@andrei8l
Copy link
Contributor Author

It seems like the quit dialog gets stuck if you send another sigint while it's open.

Thanks for catching that. It should be fixed now.

@harakka
Copy link
Member

harakka commented Oct 20, 2024

I can still reproduce it, eg. on tiles, right on the main menu before loading the game, after two sigints nothing can be selected in the dialog.

Also I noticed on the main menu, when selecting No, the game still closes.

@andrei8l andrei8l marked this pull request as draft October 20, 2024 08:08
@andrei8l
Copy link
Contributor Author

I can still reproduce it, eg. on tiles, right on the main menu before loading the game, after two sigints nothing can be selected in the dialog.

Also I noticed on the main menu, when selecting No, the game still closes.

Fixed. I'll leave this in draft for a bit, in case you find other things. Thanks a lot for testing!

@github-actions github-actions bot added the Info / User Interface Game - player communication, menus, etc. label Oct 20, 2024
@harakka
Copy link
Member

harakka commented Oct 20, 2024

Tested both tiles and curses, seems to work fine now 👍

@andrei8l andrei8l marked this pull request as ready for review October 20, 2024 10:56
@dseguin dseguin merged commit ec2416a into CleverRaven:master Oct 21, 2024
25 of 26 checks passed
@andrei8l
Copy link
Contributor Author

Thanks for merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` Info / User Interface Game - player communication, menus, etc. json-styled JSON lint passed, label assigned by github actions new contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exiting game with CTRL+C leaves console in mouse tracking mode Fix signal handler
5 participants