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

Memory leak fix on shutdown #2929

Closed
wants to merge 2 commits into from
Closed

Memory leak fix on shutdown #2929

wants to merge 2 commits into from

Conversation

plekakis
Copy link

@plekakis plekakis commented Nov 3, 2023

Fix for false-positive memory leak when using leak detection methods (such as CRT dump memory leaks).

Since we cannot rely on static de-initialisation for freeing the registry::s_instance, this is now lazily allocated instead and freed during spdlog::shutdown() function. In my test case, saves ~15 reported memory leaks.

…(such as CRT dump memory leaks).

Since we cannot rely on static de-initialisation for freeing the registry::s_instance, this is now lazily allocated instead and freed during spdlog::shutdown() function.
In my test case, saves ~15 reported memory leaks.
@gabime
Copy link
Owner

gabime commented Nov 4, 2023

I don’t know. Seems expensive to lock the singleton on each access. Double check locking with barriers could be used, but I never understood it completely. see https://preshing.com/20130930/double-checked-locking-is-fixed-in-cpp11/

@plekakis
Copy link
Author

plekakis commented Nov 4, 2023

I have updated the function to do double locking. In practice it's simple, you rely on atomics to effectively early out the expensive mutex lock path, if the instance has already been allocated.
The expensive lock path will now only occur once in the lifetime of the logger and not always.

@gabime
Copy link
Owner

gabime commented Nov 4, 2023

Shouldn’t there be std::atomic_thread_fence(std::memory_order_acquire); and std::atomic_thread_fence(std::memory_order_release); somewhere?

@gabime
Copy link
Owner

gabime commented Nov 4, 2023

I think this would break the current behavior. Currently, calling shutdown is not mandatory since the registry object would be automatically destroyed. With this pr, the registry will be never destroyed if not calling shutdown.
Please post to the v2.x branch.

@plekakis
Copy link
Author

plekakis commented Nov 4, 2023

Ah okay, I thought that shutdown was supposed to be called.
Let's close this PR, I will have a look at the v2 branch for now.

@plekakis plekakis closed this Nov 4, 2023
@gabime
Copy link
Owner

gabime commented Nov 4, 2023

@plekakis See #2113 for a discussion and a fix proposal

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

2 participants