Fix a buffer memory leak in NettyReadHandlerStateMachine #18323
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jiacheliu3
commented
Oct 25, 2023
| @@ -672,6 +672,8 @@ private void readData(RequestContext requestContext, Transition<State, TriggerEv | |||
| requestContext.getRequest()); | |||
| } | |||
| fireNext(mTriggerEventsWithParam.mOutputLengthFulfilled, requestContext); | |||
| // Release the packet as there is nothing more to read | |||
| packet.release(); | |||
There was a problem hiding this comment.
i'm not really sure if it's best to release here or actually before the fireNext(). The fireNext() might throw an exception somehow? And I figure fireNext() is not using this packet anyways.
Or would there be a better place than here, to release the packetReader.getDataBuffer() instead of releasing on the temp variable like what I'm doing? Who cleans up the resources in the context?
There was a problem hiding this comment.
I think it is OK for this case, but I am concerning if there are other Bytebuf memory leak scenarios.
dbw9580
approved these changes
Oct 26, 2023
|
alluxio-bot, merge this please |
ssz1997
pushed a commit
to ssz1997/alluxio
that referenced
this pull request
Dec 15, 2023
### What changes are proposed in this pull request? resolves Alluxio#18324 Disclaimer: I might have monkey-typed this fix but I still do not know anything about buffer ref counting. This fix does NOT make me the owner of this state machine. pr-link: Alluxio#18323 change-id: cid-eb5bde353c08d3d9bdd39da5b9caf13681bae495
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes are proposed in this pull request?
resolves #18324
Disclaimer: I might have monkey-typed this fix but I still do not know anything about buffer ref counting. This fix does NOT make me the owner of this state machine.