-
Notifications
You must be signed in to change notification settings - Fork 923
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
fix(share/ipld): Remove proofs from leaked ctx #2574
fix(share/ipld): Remove proofs from leaked ctx #2574
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2574 +/- ##
==========================================
+ Coverage 51.01% 51.07% +0.05%
==========================================
Files 158 158
Lines 10398 10428 +30
==========================================
+ Hits 5305 5326 +21
- Misses 4627 4636 +9
Partials 466 466
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume you've tested this and seen stable RAM usage on Graphana
The fact that proofs are leaking is a hint that the context is leaking. Because if the context is cleared up, the proofs would be cleared up as well. WDYT? |
I agree, Thats why PR is named for issue of leaked ctx, but not just proofs "Remove proofs from leaked ctx". Provided solution is a hotfix for latest release to prevent leak. Proper investigation of leaked ctx will take some time, since it is passed into many complex components, like ipld, bitswap, dagstore, badger. I'll create an issue to investigate where the ctx is leaked. |
(cherry picked from commit 04e2928)
(cherry picked from commit 04e2928)
(cherry picked from commit 04e2928)
(cherry picked from commit 04e2928)
Inside tee-getter ProofAdder is added to ctx to collect proofs and allow eds store to read them, without extra recompute. Ctx provided to store.Put method, will be used for register shard, that in some rare cases takes long time and returns, before result returned from operation. The provided context is still leaked to async operation, and it prevents golang GC from collecting memory allocated for ProofAdder, because there is still possibility for code to reach it.
Solution
Dagstore register shard operation does not need to access to proofs, so the associated memory could be reclaimed right after proofs are extracted from ctx. For convenience it will happen after put operation
Solves first point of #2582