Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6672 +/- ##
==========================================
- Coverage 87.49% 87.46% -0.04%
==========================================
Files 282 282
Lines 44838 44840 +2
Branches 7751 7751
==========================================
- Hits 39231 39219 -12
- Misses 5487 5501 +14
Partials 120 120
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Is it possible to test it? |
Close pipe file descriptors in periodic callback to prevent resource leaks
@meiravgri Using valgrind test I could not detect the leak, but running bash script it's possible to reproduce it and confirm the patch fix it. The script was posted to MOD-10975 |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
| # Get open file count | ||
| redis_server_pid = env.executeCommand('info')['process_id'] | ||
| open_files_before_gc = get_open_file_count(redis_server_pid) | ||
| env.assertGreater(open_files_before_gc, 0) |
There was a problem hiding this comment.
Why do we need this assertion?
There was a problem hiding this comment.
To confirm that we got the number of open files. I started implementing the test using psutil, and the test passed because it always returned 0 open files.
There was a problem hiding this comment.
How do you know that there will be open fd's?
There was a problem hiding this comment.
lsof -p <pid> counts all open file descriptors for the Redis process, including stdin, stdout, stderr, listening socket, and more.
There was a problem hiding this comment.
But you are right, the CI failed. I'll check if lsof didn't work or if I need to remove the assertion.
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin 2.8
git worktree add -d .worktree/backport-6672-to-2.8 origin/2.8
cd .worktree/backport-6672-to-2.8
git switch --create backport-6672-to-2.8
git cherry-pick -x 56f6f0ea603e422b8cf2288b328f877e4269d7da |
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin 2.6
git worktree add -d .worktree/backport-6672-to-2.6 origin/2.6
cd .worktree/backport-6672-to-2.6
git switch --create backport-6672-to-2.6
git cherry-pick -x 56f6f0ea603e422b8cf2288b328f877e4269d7da |
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin 2.10
git worktree add -d .worktree/backport-6672-to-2.10 origin/2.10
cd .worktree/backport-6672-to-2.10
git switch --create backport-6672-to-2.10
git cherry-pick -x 56f6f0ea603e422b8cf2288b328f877e4269d7da |
* Close pipe file descriptors in periodic callback to prevent resource leaks * Add open file count check in OOM test * build_utils: factor out link_static_libraries() (#6673) Will help implementing the iterator bencher. * [MOD-10771] bump svs version (#6652) * bump vecsim * vecsim with fix * test crash * code change to trigger tests * revert code chnage * ci: pin a Rust nightly version (#6513) Should prevent accidental breaking of the CI. * get open fds using psutil.num_fds() --------- Co-authored-by: Guillaume Desmottes <[email protected]> Co-authored-by: meiravgri <[email protected]> (cherry picked from commit 56f6f0e)
|
Successfully created backport PR for |
(cherry picked from commit 56f6f0e)
MOD-10975: Fix fd leak when OOM (#6672) * Close pipe file descriptors in periodic callback to prevent resource leaks * Add open file count check in OOM test * build_utils: factor out link_static_libraries() (#6673) Will help implementing the iterator bencher. * [MOD-10771] bump svs version (#6652) * bump vecsim * vecsim with fix * test crash * code change to trigger tests * revert code chnage * ci: pin a Rust nightly version (#6513) Should prevent accidental breaking of the CI. * get open fds using psutil.num_fds() --------- (cherry picked from commit 56f6f0e) Co-authored-by: nafraf <[email protected]> Co-authored-by: Guillaume Desmottes <[email protected]> Co-authored-by: meiravgri <[email protected]>
(cherry picked from commit 56f6f0e)
(cherry picked from commit 56f6f0e)
6 participants
Describe the changes in the pull request
A clear and concise description of what the PR is solving, including:
periodicCb()function returns early without closing the pipe file descriptors that were created.Missing close() calls in the out-of-memory error path were added.
Fix fd leak
Main objects this PR modified
Mark if applicable