On Wed, Jun 11, 2025 at 10:51 AM kefu chai <tchaikov@gmail.com> wrote:
On Tue, Jun 10, 2025 at 9:31 PM Casey Bodley <cbodley@redhat.com> wrote:
On Mon, Jun 9, 2025 at 10:12 AM kefu chai <tchaikov@gmail.com> wrote:
Hi folks,
I've been working on resolving memory leaks detected by ASan in our unit tests and identified a significant issue with our current approach to test instance generation.
Current Problem: Our generate_test_instance() function returns std::list<T*> for types that support encoding/decoding. While this works functionally, it creates memory management issues because:
- The lifecycle of generated test instances is inconsistently managed - Callers don't always properly clean up allocated memory - This results in numerous ASan memory leak reports in both unit tests and ceph-dencoder
Proposed Solution: I propose we modify the function to return std::list<std::shared_ptr<T>> instead of raw pointers. This change would:
- Automatically handle memory cleanup through RAII - Work correctly with types that lack move constructors - Eliminate the memory leaks we're currently seeing when testing with ASan enabled - Require updates to calling code, but provide cleaner memory semantics
could we not use `std::list<T>` and rely on emplace for types that can't be moved/copied?
Thank you for your insights. I was worried we might use it to append to an existing list. However, after reviewing the code, I confirmed that we never append to existing lists and only use it as an output parameter.
Given this, we can (and probably should) proceed with the refactoring. I'm preparing a comprehensive patch that migrates from T::generate_test_instances(std::list<T*>&) to std::list<T> T::generate_test_instances().
are you sure it matters whether we're appending? i'd expect `generate_test_instances(std::list<T>&)` to work too. if we return the list instead, each function body needs two extra lines (one to declare a `std::list<T>` on the stack, and another to return it)
For the migration strategy, while we could use SFINAE at caller sites to detect the new function signature and fall back to the old one, I think that approach might be overkill. Instead, I'm planning to implement this as a single comprehensive PR. Even though the change is substantial, it's essentially a semi-mechanical edit that should be straightforward to implement all at once.
What are your thoughts on this approach?
+1 this seems worth doing, and i'll gladly help with review
I considered adding suppression rules for the affected tests and ceph-dencoder, but that would only mask the underlying issue rather than properly addressing the memory management problem.
Next Steps: If this approach sounds reasonable, I can prepare a patch that implements this change along with the necessary caller updates.
What are your thoughts on this approach? Are there any concerns or alternative solutions you'd like to consider?
-- Regards Kefu Chai _______________________________________________ Dev mailing list -- dev@ceph.io To unsubscribe send an email to dev-leave@ceph.io
-- Regards Kefu Chai