mem: ensure Reader buffers are reused on early return#8886
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8886 +/- ##
==========================================
- Coverage 83.31% 83.14% -0.17%
==========================================
Files 414 414
Lines 32805 32821 +16
==========================================
- Hits 27330 27288 -42
- Misses 4069 4099 +30
- Partials 1406 1434 +28
🚀 New features to boost your workflow:
|
eshitachandwani
left a comment
There was a problem hiding this comment.
LGTM with minor comments. Adding @arjan-bal as a second reviewer.
| if dc != nil { | ||
| uncompressed, err := dc.Do(d.Reader()) | ||
| r := d.Reader() | ||
| defer r.Close() // ensure buffers are reused on early return |
There was a problem hiding this comment.
Can you remove the on early return part?
There was a problem hiding this comment.
Removed. Thanks for review!
There was a problem hiding this comment.
Since you are already here , do you mind wrapping this comment to make sure it is limited to 80 cols?
38a1412 to
f3e854c
Compare
| if dc != nil { | ||
| uncompressed, err := dc.Do(d.Reader()) | ||
| r := d.Reader() | ||
| defer r.Close() // ensure buffers are reused |
There was a problem hiding this comment.
I don't think we need a defer here, we can call r.Close() after dc.Do either inside the if err != nil or even outside since the implementaiton of Close is idempotent. defer statements have a very small performance cost, so we should try to avoid them on the RPC path if possible.
There was a problem hiding this comment.
I think this is even better since that way the buffers are freed ASAP. 👍
There was a problem hiding this comment.
Well, no, those are just constructors of readers. Moved to each if. I think this is worse, more fragile. But I don't mind, up to you.
6f21a70 to
015c771
Compare
If there was an error, e.g. while decompressing the data, we still want to return all buffers to the pool.
015c771 to
004f71c
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly identifies and fixes a potential buffer leak in the decompress function by ensuring that the mem.Reader is closed on error paths. This prevents buffers from being stranded if decompression fails midway.
I've suggested a small refactoring to use defer for closing the reader. This is a more idiomatic Go approach that simplifies the code and makes it more robust by guaranteeing the cleanup action is executed on all exit paths from the function blocks, not just the error paths.
arjan-bal
left a comment
There was a problem hiding this comment.
LGTM; ignoring Gemini's suggestion, using defer here is an intentional choice for performance.
If there was an error, e.g. while decompressing the data, we still want to return all buffers to the pool. RELEASE NOTES: None
If there was an error, e.g. while decompressing the data, we still want to return all buffers to the pool. RELEASE NOTES: None
If there was an error, e.g. while decompressing the data, we still want to return all buffers to the pool. RELEASE NOTES: None
If there was an error, e.g. while decompressing the data, we still want to return all buffers to the pool.
RELEASE NOTES: None