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

onShutdown execute properly when closing the server. #1277

Closed
wants to merge 1 commit into from
Closed

onShutdown execute properly when closing the server. #1277

wants to merge 1 commit into from

Conversation

EvilHero90
Copy link
Contributor

I've noticed an issue on windows (not sure if linux has that aswell)
that if you closed the console and didn't use the ingame command to set the game state to shutdown, that it wouldn't execute onShutdown() globalevent at all.
This fix worked for me.

@marksamman
Copy link
Member

While it does make sense to move onShutdown to Game::shutdown(), I prefer to leave it as it is for now because shutdown() isn't being called when you close the server the same way on a non-Windows OS. Once we've added signal handling, I'll consider merging this.

@EvilHero90
Copy link
Contributor Author

I've backtraced what you meant and you are absolutely right, do you either intend to move the entire thing to game::shutdown() and make use of #ifdef linux and #ifdef _WIN32 or what would be your solution in this case?

@marksamman
Copy link
Member

The solution right now is to leave it as-is to have consistent behavior across different operating systems; the solution once we have implemented signal handling is just to do what you did in this PR.

@EvilHero90 EvilHero90 mentioned this pull request Jul 5, 2016
@ranisalt
Copy link
Member

@marksamman as we have signal handling now maybe this PR could be rebased and merged?

@wgrr
Copy link
Contributor

wgrr commented Jan 18, 2017

@ranisalt there is only POSIX signal handlers, still left Windows console handler, reported at #1853, and maybe #2088, which is not confirmed, i think we should cover these cases also before merging this

DSpeichert added a commit to DevelopersPL/forgottenserver that referenced this pull request Jun 30, 2018
SetConsoleCtrlHandler is not needed because SIGBREAK is also sent
on window close. The process is killed when the thread handling
SIGBREAK returns. That's why we must handle it synchronously
and wait for other threads to finish first. The Windows-defined
timeout is only 5 seconds but there is no way to change that.

https://docs.microsoft.com/en-us/windows/console/ctrl-c-and-ctrl-break-signals

Fixes otland#1277
Fixes otland#1853
DSpeichert added a commit that referenced this pull request Jul 5, 2018
SetConsoleCtrlHandler is not needed because SIGBREAK is also sent
on window close. The process is killed when the thread handling
SIGBREAK returns. That's why we must handle it synchronously
and wait for other threads to finish first. The Windows-defined
timeout is only 5 seconds but there is no way to change that.

https://docs.microsoft.com/en-us/windows/console/ctrl-c-and-ctrl-break-signals

Fixes #1277
Fixes #1853
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants