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

Fix some Game members not being freed after some startup errors (regression) #14561

Merged
merged 3 commits into from
Apr 21, 2024

Conversation

grorp
Copy link
Member

@grorp grorp commented Apr 19, 2024

This PR fixes a regression from fd8e021. In that commit, I moved some freeing from Game::~Game to Game::shutdown. Now I realize that Game::shutdown isn't called in all cases, for example when a shader compilation error occurs.

Behavior in case of a shader compilation error:

Version Behavior
master with fd8e021 reverted Loading screen freezes for 5 seconds in "Initializing nodes..." phase while server shuts down. Shader error is shown afterwards.
master Shader error is shown immediately. Server is not shut down and can be joined using a second client.
this PR "Shutting down..." screen is shown for 5 seconds without freezing while server shuts down. Shader error is shown afterwards.

This PR works by always calling Game::shutdown. Somewhat related old PR: #9742

To do

This PR is a Ready for Review.

Notes:

  • I think the stuff I moved from Game::~Game to Game::shutdown could actually be moved back. However, that would mean spawning a thread (StopThread) and rendering the shutdown screen in a destructor, which feels bad. What if one of these things throws an exception? Would this be a better solution?

  • Could always calling Game::shutdown introduce new bugs?

How to test

Edit shaders to make shader compilation fail. Add this code to builtin/game/init.lua:

minetest.register_on_shutdown(function()
	print("wa wa wa waiting")
	local t = os.time() + 5
	while os.time() < t do
	end
	print("finished waiting")
end)

Verify that the behavior of different versions matches what I describe in the table above.

@sfan5
Copy link
Member

sfan5 commented Apr 19, 2024

I think a C++-y approach would be if ~Game() contained code to destroy all members.
shutdown() is just then the normal way to deinit the client, but in exceptional circumstances still nothing would leak.

@grorp
Copy link
Member Author

grorp commented Apr 19, 2024

When I first implemented your suggestion, it unfortunately made things worse: When a shader error occured, the loading screen froze again during server shutdown. To fix this, I moved some exception catching down from ClientLauncher to Game. I hope this doesn't have unexpected side effects...

Copy link
Member

@sfan5 sfan5 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.

@sfan5 sfan5 merged commit eb432d3 into minetest:master Apr 21, 2024
13 checks passed
grorp added a commit to grorp/minetest that referenced this pull request Apr 21, 2024
grorp added a commit to grorp/minetest that referenced this pull request Apr 21, 2024
@grorp grorp deleted the gam branch May 1, 2024 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants