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

unittests: fix leaks reported by ASan #605

Merged
merged 2 commits into from
May 24, 2022

Conversation

PragmaTwice
Copy link
Member

@PragmaTwice PragmaTwice commented May 24, 2022

use std::unique_ptr to avoid missing delete and dtor call

this fix can remove a large amount of leak reports from ASan (~10000+ lines in stderr), so we can check easier when other important leaks is reported.

Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PragmaTwice could you share the ASan report before and after this change?

@PragmaTwice
Copy link
Member Author

PragmaTwice commented May 24, 2022

@PragmaTwice could you share the ASan report before and after this change?

The ASan report before this is about 6MiB, I will soon find a way to upload. Seems reach the size limit in GitHub?

@PragmaTwice
Copy link
Member Author

PragmaTwice commented May 24, 2022

kvrocks-unittest-asan.log
kvrocks-unittest-asan-after.log

Seems just some error due to my poor network, not limit exceeding. Done @tisonkun.

@tisonkun tisonkun requested review from git-hulk and ShooterIT May 24, 2022 07:26
@tisonkun
Copy link
Member

@PragmaTwice looks great! It seems that this PR fixes all leaks currently reported. We should fail if there are new leaks in #599.

Copy link
Member

@git-hulk git-hulk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, cool!

@PragmaTwice
Copy link
Member Author

PragmaTwice commented May 24, 2022

@PragmaTwice looks great! It seems that this PR fixes all leaks currently reported. We should fail if there are new leaks in #599.

Seems there are also some leaks in TCL tests, I do not know the current status, maybe @git-hulk know that.
In #599, ASan is enabled in daily-ci.yaml, dont know whether it should be added to kvrocks.yaml.

ASan will slow down the program execution about x2 times, and TSan about x10 times.
Refer to https://clang.llvm.org/docs/AddressSanitizer.html and https://clang.llvm.org/docs/ThreadSanitizer.html.

@git-hulk
Copy link
Member

Yes, I can reproduce the memory leak on my side, but didn't investigate root causes now.

@tisonkun
Copy link
Member

I'm going to merge this PR.

Further discussion about #599 can go to that thread. If you'd like to investigate leak issues, it'd be better to create another issue.

@tisonkun tisonkun merged commit 62e0de9 into apache:unstable May 24, 2022
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.

3 participants