fix(buffered_reader): Fixing a memory leak in buffered reader#4638
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical memory leak within the buffered reader component. The leak occurred in scenarios where a multi-block read operation partially succeeded but then encountered an error, triggering a fallback mechanism. The fix ensures that any blocks successfully acquired before the fallback are promptly released, preventing resource accumulation and improving the stability of the reader. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request refactors the ReadAt method in internal/bufferedread/buffered_reader.go to use named return parameters and a deferred function for populating the response and handling resource cleanup. It also introduces a releaseInflightBlocks helper to ensure blocks are released during fallback scenarios. A critical issue was identified in the deferred logic where memory leaks could occur on non-fallback errors because blocks are not released, and the gcsx.Reader interface is violated by returning partial data alongside errors.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4638 +/- ##
==========================================
+ Coverage 83.62% 83.65% +0.02%
==========================================
Files 164 164
Lines 20278 20287 +9
==========================================
+ Hits 16958 16971 +13
+ Misses 2678 2676 -2
+ Partials 642 640 -2
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:
|
vadlakondaswetha
left a comment
There was a problem hiding this comment.
providing approval based on Vipins review
vadlakondaswetha
left a comment
There was a problem hiding this comment.
providing approval based on Vipins review
Fixes a memory leak in the buffered reader code during multi-block reads. Previously, if an initial block downloaded successfully but a later block failed (triggering a fallback), the reference counts on the successfully downloaded blocks were never decremented. This PR addresses the leak by catching gcsx.FallbackToAnotherReader errors and calling a new releaseInflightBlocks helper to immediately invoke callbacks and release references for any in-flight blocks.
Fixes a memory leak in the buffered reader code during multi-block reads. Previously, if an initial block downloaded successfully but a later block failed (triggering a fallback), the reference counts on the successfully downloaded blocks were never decremented. This PR addresses the leak by catching gcsx.FallbackToAnotherReader errors and calling a new releaseInflightBlocks helper to immediately invoke callbacks and release references for any in-flight blocks.
* feat: making direct-path verification non-fatal until dummy-stat calls becomes reliable (#4628) * feat: add skipDirectPathEnforcement parameter to createGRPCClientHandle to allow conditional DirectPath enforcement * chore: increase directPathDetectionTimeout from 10 seconds to 5 minutes * simplifying a bit * making direct-path verification non-fatal * removing the timeout from the client creation * minor change * removing empty line * fix(buffered_reader): Fixing a memory leak in buffered reader (#4638) Fixes a memory leak in the buffered reader code during multi-block reads. Previously, if an initial block downloaded successfully but a later block failed (triggering a fallback), the reference counts on the successfully downloaded blocks were never decremented. This PR addresses the leak by catching gcsx.FallbackToAnotherReader errors and calling a new releaseInflightBlocks helper to immediately invoke callbacks and release references for any in-flight blocks. * fix(direct path verification): Updating direct path enforcement strategy (#4635) Updating the direct path enforcement strategy: - For zonal buckets, log direct path verification status but do not block client creation on it - For non-zonal buckets, block grpc client creation on direct path verification status. In case of failure in detecting direct path status, fallback to http client would happen based on the set grpc-strategy Also, updated the timeout to 15 seconds for direct path verification. * feat(dp-check): removing older cp check utility (#4494) * feat(dp-check): removing older cp check utility * validation package is already disabled hence removing the skip --------- Co-authored-by: Prince Kumar <princer@google.com> Co-authored-by: Abhishek Gupta <abhishekmgupta@google.com>
Description
Fixes a memory leak in the buffered reader code during multi-block reads. Previously, if an initial block downloaded successfully but a later block failed (triggering a fallback), the reference counts on the successfully downloaded blocks were never decremented. This PR addresses the leak by catching gcsx.FallbackToAnotherReader errors and calling a new releaseInflightBlocks helper to immediately invoke callbacks and release references for any in-flight blocks.
Link to the issue in case of a bug fix.
b/503335735
Testing details
Any backward incompatible change? If so, please explain.