-
Notifications
You must be signed in to change notification settings - Fork 906
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
lightning-cli dev-memleak reports memleaks in lightningd.c while running clboss #7716
Comments
From what I can tell this might be a timing issue with clboss being maybe a bit slow to shut down? The way we track plugin shutdowns may have an implicit timing assumption so that if a plugin is slow it gives up and exits early, which valgrind then detects as a memleak on the shutdown tracking state. |
@cdecker I ran with VALGRIND=0, I thought this disables valgrind? |
Pretty sure this is a bug in CLN. I cannot figure out how to make this memleak go away. It seems to happen during launch rather than shutdown, around here: Line 658 in 8330e3a
|
Ok I've narrowed it down a bit further, it appears that if I turn valgrind on, it actually fixes the problem. When it's off (apparently this is by default now, IIRC valgrind used to be on by default), this section of pyln-testing runs:
So calling the |
Ok yeah confirmed, calling
..results in the above 2 memleak traces. What does this mean? |
Curiously, the tests pass when valgrind is on, but fail when valgrind is off. Does this mean that dev-memleak itself may be causing the leak?? |
I have a simple pytest for clboss and I can't seem to get it to pass, even with the most basic testing code.
Here's the test:
And here's the output:
If I don't include the plugin, the test passes, so it doesn't seem that there's anything wrong with the framework. It just seems that CLN leaks memory somehow when running with CLBOSS. I'm not sure why this would happen, as none of the other plugins in the repo have this issue when running nightly in the CI.
Anyone got ideas? @rustyrussell @cdecker @ShahanaFarooqui @ksedgwic @vincenzopalazzo
The text was updated successfully, but these errors were encountered: