-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
bugfix: netty memory leak #1333
Conversation
netty 4.1.24 LengthFieldBasedFrameDecoder LEAK netty/netty#8140
Codecov Report
@@ Coverage Diff @@
## develop #1333 +/- ##
=============================================
+ Coverage 48.47% 48.63% +0.15%
Complexity 1645 1645
=============================================
Files 332 332
Lines 11503 11534 +31
Branches 1391 1428 +37
=============================================
+ Hits 5576 5609 +33
- Misses 5292 5302 +10
+ Partials 635 623 -12
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
ByteBuf frame = (ByteBuf) decoded; | ||
return decodeFrame(frame); | ||
}catch(Exception e){ | ||
throw e; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please print some log
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good job.
}catch(Exception e){ | ||
throw e; | ||
}finally { | ||
in.release(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not release in
but frame
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree,in
can be released in ByteToMessageDecoder.channelRead
, but frame
allocate in LengthFieldBasedFrameDecoder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change frame.release and add error logger
Ⅰ. Describe what this PR did
Ⅱ. Does this pull request fix one issue?
fix #1304 #1318
Ⅲ. Why don't you add test cases (unit test/integration test)?
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews