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
[BUG] Use local GC ctx in periodic callback #2962
Conversation
This pull request introduces 1 alert when merging f711348 into d5a17d3 - view on LGTM.com new alerts:
|
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.
👍🏼
This pull request introduces 1 alert when merging 7c17188 into d5a17d3 - view on LGTM.com new alerts:
|
Codecov Report
@@ Coverage Diff @@
## master #2962 +/- ##
==========================================
+ Coverage 81.92% 81.94% +0.01%
==========================================
Files 180 180
Lines 29264 29452 +188
==========================================
+ Hits 23974 24133 +159
- Misses 5290 5319 +29
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
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.
Looks good, why comment a test?
src/gc.c
Outdated
RedisModuleCtx* ctx; | ||
switch (RSGlobalConfig.gcPolicy) { | ||
case GCPolicy_Fork: | ||
ctx = ((ForkGC *)(gc->gcCtx))->ctx; |
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 believe there is no harm in creating ThreadSafeCtx anyway each time.
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.
Replaced with RedisModule_GetThreadSafeContext
and freed.
Let's push this.
@@ -79,7 +79,7 @@ static void threadCallback(void* data) { | |||
GCTask* task= data; | |||
GCContext* gc = task->gc; | |||
RedisModuleBlockedClient* bc = task->bClient; | |||
RedisModuleCtx* ctx = RSDummyContext; | |||
RedisModuleCtx* ctx = RedisModule_GetThreadSafeContext(NULL); | |||
|
|||
if (gc->stopped) { | |||
// if the client is blocked, lets release it |
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.
Free ctx here
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.
Last comment, other then this looks good
* Use local GC ctx and add logs * get the right ctx * per review * add test * Skip GC on cluster and temporarily comment out part of testMultiTextNested * streamline code * test fixed in #2967 * fix leak Co-authored-by: oshadmi <omer.shadmi@redislabs.com> Co-authored-by: Omer Shadmi <76992134+oshadmi@users.noreply.github.com> (cherry picked from commit 3f7c8db)
* Use local GC ctx and add logs * get the right ctx * per review * add test * Skip GC on cluster and temporarily comment out part of testMultiTextNested * streamline code * test fixed in #2967 * fix leak Co-authored-by: oshadmi <omer.shadmi@redislabs.com> Co-authored-by: Omer Shadmi <76992134+oshadmi@users.noreply.github.com> (cherry picked from commit 3f7c8db)
* Use local GC ctx and add logs * get the right ctx * per review * add test * Skip GC on cluster and temporarily comment out part of testMultiTextNested * streamline code * test fixed in #2967 * fix leak Co-authored-by: oshadmi <omer.shadmi@redislabs.com> Co-authored-by: Omer Shadmi <76992134+oshadmi@users.noreply.github.com> (cherry picked from commit 3f7c8db)
* Use local GC ctx and add logs * get the right ctx * per review * add test * Skip GC on cluster and temporarily comment out part of testMultiTextNested * streamline code * test fixed in #2967 * fix leak Co-authored-by: oshadmi <omer.shadmi@redislabs.com> Co-authored-by: Omer Shadmi <76992134+oshadmi@users.noreply.github.com>
Using
RSDummeyCtx
withAutoMemory
caused memory corruption as Redis maintains a list of freed pointers.When RM_RetainString is called on a pointer in the auto memory pointer list, Redis does not increase the ref count, which causes
double free
.MOD-3951
Fixes #2766