-
Notifications
You must be signed in to change notification settings - Fork 896
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
Fix memory leak when reading entry but the connection disconnected. #3528
Conversation
@@ -145,12 +152,12 @@ protected void sendResponseAndWait(int rc, Object response, OpStatsLogger statsL | |||
try { | |||
ChannelFuture future = channel.writeAndFlush(response); | |||
if (!channel.eventLoop().inEventLoop()) { | |||
future.await(); |
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.
what's the difference in this case ?
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.
await()
won't throw the exception out. We'd better catch the exception and log it in debug mode to help debug issues.
void retain() { | ||
} | ||
|
||
void 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.
what about keeping boolean release()
IIUC your problem is about implementing ReferenceCounted
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.
It has conflict with the release() interface in ReferenceCounted
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.
Yeah. I think we can keep boolean release()
. Because ReferenceCounted
also has the same method.
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.
👍
@@ -445,7 +443,7 @@ void recycle() { | |||
/** | |||
* A request that reads data. | |||
*/ | |||
class ReadResponse extends Response { | |||
class ReadResponse extends Response implements ReferenceCounted { |
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.
why do we need to implement ReferenceCounted ?
can you add a comment in the JavaDoc ?
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.
#3527 will give more context.
@horizonzy Great Catch! |
channel.writeAndFlush(response, channel.voidPromise()); | ||
ChannelPromise promise = channel.newPromise().addListener(future -> { | ||
if (!future.isSuccess()) { | ||
logger.debug("Netty channel write exception. ", future.cause()); |
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.
Why not log it as an error?
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.
When this error occurs, it means that the client has already close. Log error is unnecessary, the other log will show the connection already close.
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.
I think this need to print the error
logger.error("Netty channel write exception. ", future.cause());
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PacketProcessorBase.java
Show resolved
Hide resolved
void retain() { | ||
} | ||
|
||
void 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.
Yeah. I think we can keep boolean release()
. Because ReferenceCounted
also has the same method.
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
rerun failure checks |
ping @eolivelli PTAL again, thanks |
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
@StevenLuMT @zymap do you have other comments ? |
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.
Nicely done.
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
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
…pache#3528) * Fix direct memory leak problem (cherry picked from commit 30bdedc)
…pache#3528) * Fix direct memory leak problem (cherry picked from commit 30bdedc)
…pache#3528) * Fix direct memory leak problem (cherry picked from commit 30bdedc) (cherry picked from commit 9329e3c)
…ected. (apache#3528)" This reverts commit cef988f.
ChannelPromise promise = channel.newPromise().addListener(future -> { | ||
if (!future.isSuccess()) { | ||
logger.debug("Netty channel write exception. ", future.cause()); | ||
} | ||
}); |
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.
This will cause unnecessary object creation. If the only purpose is to log exceptions, channel.voidPromise()
already takes care of that.
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.
Thanks for reminder.
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.
It has been fixed here: #3733
…pache#3528) * Fix direct memory leak problem (cherry picked from commit 30bdedc) (cherry picked from commit 9329e3c)
…pache#3528) * Fix direct memory leak problem (cherry picked from commit 30bdedc) (cherry picked from commit 9329e3c)
…pache#3528) * Fix direct memory leak problem
Descriptions of the changes in this PR:
Fixes-#3527