-
Notifications
You must be signed in to change notification settings - Fork 283
Fix memory leak for batch delete #3161
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
Conversation
| } finally { | ||
| if (byteBuffer != null) { | ||
| byteBuffer.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.
Why are we only seeing this issue now? Were we relying on the garbage collector? Is there increased load in batch delete requests that prevents the garbage collector from keeping up?
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 is direct memory leak. so no gc, we have to release them.
we only see bunch of these when we enable the paranoid mode and the direct memory usage goes up to 20G.
we will keep investigate other leaks
| throw new RestServiceException("failed to deserialize", e, RestServiceErrorCode.BadRequest); | ||
| } finally { | ||
| if (byteBuffer != null) { | ||
| byteBuffer.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.
does consumeContentAsBuf increase ref count? is that why release is being done here? is channel.close() getting called correctly?
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 looks like consumeContentAsByteBuf transfers ownership. can we refactor that method to be safe? something like:
public byte[] consumeContentAsBytes() {
ByteBuf buf = null;
try {
buf = // consumeContentAsByteBuf() code
byte[] data = new byte[buf.readableBytes()];
buf.readBytes(data);
return data;
} finally {
if (buf != null) { buf.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.
done
78ca9c4 to
c55277b
Compare
c55277b to
bd65901
Compare
Summary
Fix memory leak for batch delete
Testing Done
unit test