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

Is CI not checking for memory leaks? #7565

Closed
Lagrang3 opened this issue Aug 13, 2024 · 2 comments · Fixed by #7573
Closed

Is CI not checking for memory leaks? #7565

Lagrang3 opened this issue Aug 13, 2024 · 2 comments · Fixed by #7573
Milestone

Comments

@Lagrang3
Copy link
Collaborator

Lagrang3 commented Aug 13, 2024

After this issue #7560, it occurred to me that I have been pushing code that the CI have not checked for memory leaks.
As far as I know, I can enforce memory leak detection by setting the environment variable LIGHTNING_DEV_MEMLEAK, at least
that is what is written in common/memleak.c.

Locally I run pyln tests like this:

LIGHTNINGD_DEV_MEMLEAK=1 pytest tests/test_renepay.py -x

But CI is clearly not doing this check for memory leaks, otherwise #7560 would have been caught by it during PR review.

@Lagrang3 Lagrang3 changed the title I the CI not checking for memory leaks? Is CI not checking for memory leaks? Aug 13, 2024
@rustyrussell
Copy link
Contributor

Checking...

rustyrussell added a commit to rustyrussell/lightning that referenced this issue Aug 14, 2024
Commit 901342b ("pyln-testing: require
explicit pattern for BROKEN messages.") changed to a manual scan of
logs rather than using is_in_log, so it needs to manually refresh,
otherwise we miss final log messages.

This causes us to often miss memleak messages, which are printed
only in the exit path!

Reported-by: Lagrang3
Fixes: ElementsProject#7565
Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
Copy link
Contributor

Good catch!

@rustyrussell rustyrussell added this to the v24.08 milestone Aug 14, 2024
rustyrussell added a commit to rustyrussell/lightning that referenced this issue Aug 14, 2024
Commit 901342b ("pyln-testing: require
explicit pattern for BROKEN messages.") changed to a manual scan of
logs rather than using is_in_log, so it needs to manually refresh,
otherwise we miss final log messages.

This causes us to often miss memleak messages, which are printed
only in the exit path!

Reported-by: Lagrang3
Fixes: ElementsProject#7565
Signed-off-by: Rusty Russell <[email protected]>
rustyrussell added a commit to rustyrussell/lightning that referenced this issue Aug 15, 2024
Commit 901342b ("pyln-testing: require
explicit pattern for BROKEN messages.") changed to a manual scan of
logs rather than using is_in_log, so it needs to manually refresh,
otherwise we miss final log messages.

This causes us to often miss memleak messages, which are printed
only in the exit path!

Reported-by: Lagrang3
Fixes: ElementsProject#7565
Signed-off-by: Rusty Russell <[email protected]>
rustyrussell added a commit to rustyrussell/lightning that referenced this issue Aug 15, 2024
Commit 901342b ("pyln-testing: require
explicit pattern for BROKEN messages.") changed to a manual scan of
logs rather than using is_in_log, so it needs to manually refresh,
otherwise we miss final log messages.

This causes us to often miss memleak messages, which are printed
only in the exit path!

Reported-by: Lagrang3
Fixes: ElementsProject#7565
Signed-off-by: Rusty Russell <[email protected]>
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 a pull request may close this issue.

2 participants