Skip to content
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

Handle a leak when thrift requests are aborted early #5535

Merged
merged 1 commit into from Apr 3, 2024

Conversation

jrhee17
Copy link
Contributor

@jrhee17 jrhee17 commented Mar 27, 2024

Motivation:

We received a report where thrift was leaking pooled byte buffers.
We use pooled byte buffers to create the thrift request in THttpClientDelegate assuming
that it will be released when sent on the wire for normal cases.

However, if any requests don't have a chance to reach the wire, they will leak the byte buffer.
I believe it is reasonable to assume that requests not decoded properly should be cleaned up. Hence, I propose we do an extra req.abort inside handlePreDecodeException.

Modifications:

  • Modify handlePreDecodeException to accept an additional HttpRequest parameter
  • Abort the request with the exception if exists

Result:

@jrhee17 jrhee17 added the defect label Mar 27, 2024
@jrhee17 jrhee17 added this to the 1.28.0 milestone Mar 27, 2024
Copy link

codecov bot commented Mar 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.06%. Comparing base (fadd348) to head (28a1938).
Report is 1 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #5535      +/-   ##
============================================
+ Coverage     74.04%   74.06%   +0.02%     
- Complexity    20851    20861      +10     
============================================
  Files          1807     1807              
  Lines         76757    76760       +3     
  Branches       9792     9793       +1     
============================================
+ Hits          56833    56852      +19     
+ Misses        15292    15283       -9     
+ Partials       4632     4625       -7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jrhee17 jrhee17 marked this pull request as ready for review March 27, 2024 05:58
Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

👍 👍 👍

Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

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

Thanks, @jrhee17!

@jrhee17 jrhee17 merged commit 3fc6f8f into line:main Apr 3, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants