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

Memory issue of kvrocks2redis detected by ASan/TSan #2195

Open
1 of 2 tasks
Zakelly opened this issue Mar 23, 2024 · 12 comments
Open
1 of 2 tasks

Memory issue of kvrocks2redis detected by ASan/TSan #2195

Zakelly opened this issue Mar 23, 2024 · 12 comments
Assignees
Labels
bug type bug crash type crash

Comments

@Zakelly
Copy link
Contributor

Zakelly commented Mar 23, 2024

Search before asking

  • I had searched in the issues and found no similar issues.

Motivation

Currently, the Kvrocks2redis CI fails with TSAN/ASAN or in macos env. An example for this: https://github.com/Zakelly/kvrocks/actions/runs/8344973371/job/22838786340
The value mismatch in destination redis server. More investigation is needed.

Solution

No response

Are you willing to submit a PR?

  • I'm willing to submit a PR!
@Zakelly Zakelly added the enhancement type enhancement label Mar 23, 2024
@jihuayu
Copy link
Member

jihuayu commented Mar 23, 2024

Thank you @Zakelly !
The action log will be deleted by GitHub after some days. Can you download the log file and upload?

@Zakelly
Copy link
Contributor Author

Zakelly commented Mar 24, 2024

@jihuayu Sure thing. Log attached.
logs_21881009491.zip

@PragmaTwice
Copy link
Member

IMHO we should spend some time on investigating this issue and find a solution before next release. cc @git-hulk

@PragmaTwice PragmaTwice added bug type bug and removed enhancement type enhancement labels Apr 20, 2024
@git-hulk
Copy link
Member

git-hulk commented Apr 20, 2024

@PragmaTwice I may take some time to investigate this after resolving #2253, and it'd be great if other guys would like to dive deep into this issue.

@PragmaTwice PragmaTwice changed the title Kvrocks2redis CI failed with TSAN/ASAN or in macos env Memory issue of kvrocks2redis detected by ASan/TSan Apr 20, 2024
@PragmaTwice
Copy link
Member

@PragmaTwice I may take some time to investigate this after resolving #2253, and it'd be great if other guys would like to dive deep into this issue.

I think this issue is related to #1988 which makes kvrocks2redis crash.
It is important to keep kvrocks2redis usable, without crashing.

@PragmaTwice PragmaTwice added the crash type crash label Apr 20, 2024
@AntiTopQuark
Copy link
Contributor

AntiTopQuark commented Apr 22, 2024

@PragmaTwice I may take some time to investigate this after resolving #2253, and it'd be great if other guys would like to dive deep into this issue.

@git-hulk
Hi, have you started studying this core? If not I kind of want to try it.

@git-hulk
Copy link
Member

@AntiTopQuark Not yet, thank you!

@AntiTopQuark
Copy link
Contributor

@Zakelly Hi, I've investigated this issue and successfully reproduced it on my computer.

I suspect it's not a memory issue, but rather that compiling with ASAN and TSAN significantly impacts the performance of kvrocks2redis. This leads to slower data synchronization in kvrocks2redis.

And a simple sleep(0.02)(url) in the check_consistency.py test script might not give Redis enough time to sync the corresponding results, resulting in execution errors.

As shown in the diagram below, although the test failed, the corresponding keys and values can still be found in Redis afterward.
image

Or we can try to make the test script sleep longer?

@AntiTopQuark
Copy link
Contributor

@Zakelly Hi, I've investigated this issue and successfully reproduced it on my computer.

I suspect it's not a memory issue, but rather that compiling with ASAN and TSAN significantly impacts the performance of kvrocks2redis. This leads to slower data synchronization in kvrocks2redis.

And a simple sleep(0.02)(url) in the check_consistency.py test script might not give Redis enough time to sync the corresponding results, resulting in execution errors.

As shown in the diagram below, although the test failed, the corresponding keys and values can still be found in Redis afterward. image

Or we can try to make the test script sleep longer?

cc @git-hulk

@git-hulk
Copy link
Member

@AntiTopQuark Thanks for your great investigation. Can we relax the check condition? like check it N times with a fixed interval and return once it's passed. cc @PragmaTwice What do you think?

@PragmaTwice
Copy link
Member

Yeah we can adjust 0.02 to see if it works.

@Zakelly
Copy link
Contributor Author

Zakelly commented May 10, 2024

Happy to see the great investigation! Thanks @AntiTopQuark

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug type bug crash type crash
Projects
Status: In Progress
Development

No branches or pull requests

5 participants