fix bugs detected by doubao #898
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes bugs in the getty remoting layer by implementing proper session cleanup, resource management, and reconnection functionality with exponential backoff.
- Adds proper session resource cleanup when sessions close or encounter errors
- Implements proper cleanup of message futures to prevent memory leaks
- Introduces reconnection logic with exponential backoff strategy for improved client reliability
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/remoting/getty/session_manager.go | Adds cleanupSessionResources method to properly clean up session attributes |
| pkg/remoting/getty/listener.go | Adds cleanupSession calls in OnError and OnClose handlers |
| pkg/remoting/getty/getty_remoting.go | Fixes memory leak by properly deleting processed message futures |
| pkg/remoting/getty/getty_client.go | Adds reconnection logic with exponential backoff functionality |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
pkg/remoting/getty/getty_client.go
Outdated
| func (client *GettyRemotingClient) reconnectWithBackoff(session getty.Session, maxRetries int) { | ||
| ctx, cancel := context.WithCancel(context.Background()) | ||
| defer cancel() | ||
| cfg := backoff.Config{ | ||
| MinBackoff: 1 * time.Second, | ||
| MaxBackoff: 60 * time.Second, | ||
| MaxRetries: maxRetries, | ||
| } | ||
| backoffInstance := backoff.New(ctx, cfg) | ||
|
|
||
| log.Infof("Starting reconnection attempts with backoff for session: %s", session.Stat()) | ||
|
|
||
| for backoffInstance.Ongoing() { | ||
| backoffInstance.Wait() | ||
|
|
||
| log.Infof("Reconnection attempt %d for session: %s", backoffInstance.NumRetries(), session.Stat()) | ||
| } | ||
|
|
||
| if err := backoffInstance.Err(); err != nil { | ||
| log.Errorf("Reconnection failed after %d attempts: %v", backoffInstance.NumRetries(), err) | ||
| } | ||
| } |
There was a problem hiding this comment.
The reconnectWithBackoff method performs only logging but doesn't actually attempt to reconnect. The loop should include actual reconnection logic instead of just waiting and logging.
| func (g *gettyClientHandler) cleanupSession(session getty.Session) { | ||
| session.RemoveAttribute(heartBeatRetryTimesKey) | ||
| log.Debugf("Cleaned up resources for closing session: %s", session.Stat()) | ||
| } |
There was a problem hiding this comment.
The cleanupSession method in gettyClientHandler duplicates the same logic as cleanupSessionResources in SessionManager. Consider consolidating this cleanup logic into a single reusable function to avoid code duplication.
| func (g *gettyClientHandler) cleanupSession(session getty.Session) { | |
| session.RemoveAttribute(heartBeatRetryTimesKey) | |
| log.Debugf("Cleaned up resources for closing session: %s", session.Stat()) | |
| } | |
| // cleanupSessionResources removes session attributes and logs cleanup. | |
| func cleanupSessionResources(session getty.Session) { | |
| session.RemoveAttribute(heartBeatRetryTimesKey) | |
| log.Debugf("Cleaned up resources for closing session: %s", session.Stat()) | |
| } | |
| func (g *gettyClientHandler) cleanupSession(session getty.Session) { | |
| cleanupSessionResources(session) | |
| } |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #898 +/- ##
=======================================
Coverage 54.52% 54.53%
=======================================
Files 209 209
Lines 12913 12985 +72
=======================================
+ Hits 7041 7081 +40
- Misses 5372 5403 +31
- Partials 500 501 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
pkg/remoting/getty/getty_client.go
Outdated
| import ( | ||
| "context" | ||
| "fmt" | ||
| getty "github.com/apache/dubbo-getty" |
|
would you like to add some uts for this PR? |
OK, I will improve the unit test to verify the newly added reconnection method |
I removed the reconnect method and only kept the resource cleanup part. |
| // messageFuture.Err = rpcMessage.Err | ||
| messageFuture.Done <- struct{}{} | ||
| // client.msgFutures.Delete(rpcMessage.RequestID) | ||
| g.futures.Delete(rpcMessage.ID) |
There was a problem hiding this comment.
Maybe ‘g.futures’ is nil？
| cleanupSession(session) | ||
| } | ||
| func cleanupSession(session getty.Session) { | ||
| session.RemoveAttribute(heartBeatRetryTimesKey) |
There was a problem hiding this comment.
Pls add check for ‘ session’
4 participants
What this PR does:
for #879
Which issue(s) this PR fixes:
fix some issue about getty