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 memory leak in EmergeManager #12129

Merged
merged 1 commit into from Mar 14, 2022

Conversation

setupminimal
Copy link
Contributor

EmergeManager keeps a pointer to a BiomeGen that it creates and then hands (a copy of) out to each worker thread, but never deletes the BiomeGen.

To do

This PR is Ready for Review.

How to test

This fix can be verified with Valgrind by creating, entering, and then leaving a world.

@sfan5 sfan5 changed the title EmergeManager memory leak Fix memory leak in EmergeManager Mar 12, 2022
src/emerge.cpp Outdated Show resolved Hide resolved
@sfan5 sfan5 added the Bugfix 🐛 PRs that fix a bug label Mar 12, 2022
@sfan5
Copy link
Member

sfan5 commented Mar 12, 2022

If you plan on fixing more memory leaks it'd be okay to collect them for one big PR, by the way. No need to open one for each.

EmergeManager keeps a copy of the BiomeGen that it creates, but
never deletes it.
@setupminimal
Copy link
Contributor Author

I've just been tackling the next error Valgrind turns up each time I have a bit of free time, but that's a fair point.

I have one more pull request along these lines in the work, but it's a much larger refactor to fix an ambiguous ownership issue, and I haven't spotted any other problems with Valgrind. I expect that I'll probably have it sorted out sometime next week or so.

And I do appreciate your help in reviewing and merging these pull requests -- tracking down these issues has been really fun and satisfying. I'll make sure to consolidate smaller pull requests in the future.

@sfan5 sfan5 added Trivial The change is a trivial bug fix, documentation or maintenance change, as per the Git Guidelines One approval ✅ ◻️ labels Mar 12, 2022
@sfan5 sfan5 merged commit e54f5e5 into minetest:master Mar 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bugfix 🐛 PRs that fix a bug Trivial The change is a trivial bug fix, documentation or maintenance change, as per the Git Guidelines >= Two approvals ✅ ✅
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants