Skip to content

Conversation

@uriyage
Copy link
Contributor

@uriyage uriyage commented Sep 16, 2025

Set free method for deferred_reply list to properly clean up
ClientReplyValue objects when the list is destroyed

This was introduced in #1819.

@codecov
Copy link

codecov bot commented Sep 16, 2025

Codecov Report

❌ Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.22%. Comparing base (fab2a12) to head (5731d8b).
⚠️ Report is 2 commits behind head on unstable.

Files with missing lines Patch % Lines
src/networking.c 0.00% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #2615      +/-   ##
============================================
- Coverage     72.26%   72.22%   -0.04%     
============================================
  Files           127      127              
  Lines         70826    70828       +2     
============================================
- Hits          51180    51159      -21     
- Misses        19646    19669      +23     
Files with missing lines Coverage Δ
src/networking.c 88.22% <0.00%> (-0.06%) ⬇️

... and 14 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ranshid ranshid added the bug Something isn't working label Sep 16, 2025
@ranshid ranshid moved this to To be backported in Valkey 8.1 Sep 16, 2025
@ranshid ranshid moved this to In Progress in Valkey 9.0 Sep 16, 2025
@ranshid
Copy link
Member

ranshid commented Sep 16, 2025

So this case of memory leak is when the client will be freed before the blocked command is sent over the network correct? Can you explain the exact path that can lead to such a case? also maybe we should explain more on the top comment.

I also think we should include a test for this case as there was no test of that path added.

@ranshid
Copy link
Member

ranshid commented Sep 16, 2025

@yairgott can you PTAL?

Copy link
Contributor

@yairgott yairgott left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this!

IMO, It would be more intuitive if the default free function of a list is zfree while listSetFreeMethod is used to override.

@ranshid ranshid merged commit 80bbbcf into valkey-io:unstable Sep 17, 2025
62 of 63 checks passed
@github-project-automation github-project-automation bot moved this from In Progress to Done in Valkey 9.0 Sep 17, 2025
rjd15372 pushed a commit to rjd15372/valkey that referenced this pull request Sep 19, 2025
Set free method for deferred_reply list to properly clean up 
ClientReplyValue objects when the list is destroyed

Signed-off-by: Uri Yagelnik <[email protected]>
rjd15372 pushed a commit that referenced this pull request Sep 23, 2025
Set free method for deferred_reply list to properly clean up 
ClientReplyValue objects when the list is destroyed

Signed-off-by: Uri Yagelnik <[email protected]>
@ranshid ranshid moved this from To be backported to In Progress in Valkey 8.1 Sep 30, 2025
ranshid pushed a commit to ranshid/valkey that referenced this pull request Sep 30, 2025
Set free method for deferred_reply list to properly clean up 
ClientReplyValue objects when the list is destroyed

Signed-off-by: Uri Yagelnik <[email protected]>
ranshid pushed a commit to ranshid/valkey that referenced this pull request Sep 30, 2025
Set free method for deferred_reply list to properly clean up 
ClientReplyValue objects when the list is destroyed

Signed-off-by: Uri Yagelnik <[email protected]>
@ranshid ranshid moved this from In Progress to 8.1.4 in Valkey 8.1 Sep 30, 2025
@ranshid ranshid moved this from 8.1.4 to To be backported in Valkey 8.1 Sep 30, 2025
zuiderkwast pushed a commit that referenced this pull request Oct 1, 2025
Set free method for deferred_reply list to properly clean up 
ClientReplyValue objects when the list is destroyed

Signed-off-by: Uri Yagelnik <[email protected]>
@zuiderkwast zuiderkwast moved this from To be backported to 8.1.4 in Valkey 8.1 Oct 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

Status: 8.1.4
Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants