-
Notifications
You must be signed in to change notification settings - Fork 286
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:Reclaim the heartbeat response message to avoid memory leakage of GettyRemoting.futures #665
Conversation
@@ -38,5 +38,9 @@ func (f *clientHeartBeatProcessor) Process(ctx context.Context, rpcMessage messa | |||
log.Debug("received PONG from {}", ctx) | |||
} | |||
} | |||
msgFuture := getty.GetGettyRemotingInstance().GetMessageFuture(rpcMessage.ID) |
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.
use defer or better?
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.
正常移除就行吧
Quality Gate passedIssues Measures |
Quality Gate passedIssues Measures |
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.
LGTE
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. The optimization suggested is to handle RemoveMessageFuture in a centralized manner, rather than processing it in each type of message.
What this PR does:
Reclaim the heartbeat response message to avoid memory leakage of GettyRemoting.futures
Which issue(s) this PR fixes:
NONE
Special notes for your reviewer:
When sending a message, in GettyRemoting.futures, added a new records, but in dealing with a heartbeat message receipt, no call GettyRemoting.RemoveMessageFuture method, This leads to an increasing amount of data in GettyRemoting.futures, which eventually leads to oom
Does this PR introduce a user-facing change?:
NONE