On Thu, Jun 12, 2025 at 1:41 AM Casey Bodley <cbodley@xxxxxxxxxx> wrote:
On Wed, Jun 11, 2025 at 10:51 AM kefu chai <tchaikov@xxxxxxxxx> wrote:
>
>
>
> On Tue, Jun 10, 2025 at 9:31 PM Casey Bodley <cbodley@xxxxxxxxxx> wrote:
>>
>> On Mon, Jun 9, 2025 at 10:12 AM kefu chai <tchaikov@xxxxxxxxx> 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 don't think it matters, at least for now.
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)
yeah, agreed. but i have changed more than 300 instances of the functions to return std::list<T> so far, there are 600 more to go. i plan to finish all of them in this week. if you think it's important to save these two lines in the function body. i will rewrork them in the following days.
>
> 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
thank you, Casey!
>
>
>>
>> >
>> > 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@xxxxxxx
>> > To unsubscribe send an email to dev-leave@xxxxxxx
>>
>
>
> --
> Regards
> Kefu Chai
--
Regards
Kefu Chai
Kefu Chai
_______________________________________________ Dev mailing list -- dev@xxxxxxx To unsubscribe send an email to dev-leave@xxxxxxx
