AlexStocks commented on code in PR #898:
URL:
https://github.com/apache/incubator-seata-go/pull/898#discussion_r3288449445
##########
pkg/remoting/getty/listener.go:
##########
@@ -56,6 +56,10 @@ func GetGettyClientHandlerInstance() *gettyClientHandler {
func (g *gettyClientHandler) OnOpen(session getty.Session) error {
log.Infof("Open new getty session ")
+ if session == nil {
Review Comment:
[P1] OnOpen 签名约定返回 error 通知调用方失败，但 session 为 nil 时返回 nil，调用方认为打开成功，实际无
session 被注册。违反 Go 错误处理惯例。
建议：`return fmt.Errorf("OnOpen called with nil session")`。如果 nil session
是不可能场景则应 panic 而非静默返回。
##########
pkg/remoting/getty/listener.go:
##########
@@ -77,15 +81,21 @@ func (g *gettyClientHandler) OnOpen(session getty.Session)
error {
func (g *gettyClientHandler) OnError(session getty.Session, err error) {
log.Infof("session{%s} got error{%v}, will be closed.", session.Stat(),
err)
+ g.cleanupSession(session)
sessionManager.releaseSession(session)
}
func (g *gettyClientHandler) OnClose(session getty.Session) {
log.Infof("session{%s} is closing......", session.Stat())
+ g.cleanupSession(session)
sessionManager.releaseSession(session)
Review Comment:
[P0] OnClose 同样未做 nil 检查，直接调用 session.Stat()。与 OnError 同一问题，panic 风险。
##########
pkg/remoting/getty/listener.go:
##########
@@ -77,15 +81,21 @@ func (g *gettyClientHandler) OnOpen(session getty.Session)
error {
func (g *gettyClientHandler) OnError(session getty.Session, err error) {
log.Infof("session{%s} got error{%v}, will be closed.", session.Stat(),
err)
+ g.cleanupSession(session)
Review Comment:
[P0] OnError 直接调用 session.Stat() 未做 nil 检查。本 PR 在
OnOpen/OnMessage/OnCron/transferHeartBeat 中都加了 nil 检查，但 OnError 和 OnClose
遗漏了。如果 session 可能为 nil（这是本 PR 自身的防御前提），OnError 是异常回调，更容易出现边界情况，此处将直接 nil
dereference panic。
建议：
```go
func (g *gettyClientHandler) OnError(session getty.Session, err error) {
if session == nil {
log.Warn("OnError called with nil session")
return
}
// ...
}
```
OnClose 同理。
##########
pkg/remoting/getty/getty_remoting.go:
##########
@@ -130,6 +130,7 @@ func (g *GettyRemoting) NotifyRpcMessageResponse(rpcMessage
message.RpcMessage)
// messageFuture.Err = rpcMessage.Err
messageFuture.Done <- struct{}{}
// client.msgFutures.Delete(rpcMessage.RequestID)
+ g.futures.Delete(rpcMessage.ID)
Review Comment:
[P1] NotifyRpcMessageResponse 正确补充了 futures.Delete，但超时路径 syncCallback 中的
RemoveMergedMessageFuture 之后**未调用** RemoveMessageFuture（即
futures.Delete）。响应超时后到达时，NotifyRpcMessageResponse 仍找到 future 并写入 Done channel，但
syncCallback 已放弃等待，造成 future 泄漏。
建议在 syncCallback 超时分支增加 `g.gettyRemoting.futures.Delete(reqMsg.ID)`，并将 Done
channel 改为带缓冲 `make(chan struct{}, 1)` 防止写入方阻塞。
##########
pkg/remoting/getty/listener.go:
##########
@@ -56,6 +56,10 @@ func GetGettyClientHandlerInstance() *gettyClientHandler {
func (g *gettyClientHandler) OnOpen(session getty.Session) error {
log.Infof("Open new getty session ")
Review Comment:
[P1] log.Infof 在 nil 检查之前执行，当 session 为 nil 时仍输出"打开新 session"的误导性日志。应将日志移到
nil 检查之后。
##########
pkg/remoting/getty/session_manager.go:
##########
@@ -212,6 +216,7 @@ func (g *SessionManager) releaseSession(session
getty.Session) {
m, _ := g.serverSessions.LoadOrStore(session.RemoteAddr(),
&sync.Map{})
sMap := m.(*sync.Map)
sMap.Delete(session)
+ g.cleanupSessionResources(session)
Review Comment:
[P1] releaseSession 中 cleanupSessionResources 调用 session.Stat() 和
session.RemoveAttribute()，但 releaseSession 无锁保护。如果另一个 goroutine 并发关闭同一
session，存在 TOCTOU 竞态。
建议：移除 cleanupSession 中的 session.Stat() 调用，或在 releaseSession 入口缓存 session
状态信息。
##########
pkg/remoting/getty/listener.go:
##########
@@ -143,3 +160,15 @@ func (g *gettyClientHandler) RegisterProcessor(msgType
message.MessageType, proc
g.processorMap[msgType] = processor
}
}
Review Comment:
[P2] cleanupSession 为包级函数，与 gettyClientHandler.cleanupSession
方法同名，降低可读性且违反封装原则。建议重命名为 removeHeartBeatRetryAttribute 或将逻辑内联到
SessionManager.cleanupSessionResources。
##########
pkg/remoting/getty/listener.go:
##########
@@ -77,15 +81,21 @@ func (g *gettyClientHandler) OnOpen(session getty.Session)
error {
func (g *gettyClientHandler) OnError(session getty.Session, err error) {
log.Infof("session{%s} got error{%v}, will be closed.", session.Stat(),
err)
+ g.cleanupSession(session)
sessionManager.releaseSession(session)
Review Comment:
[P1] cleanupSession 被双重调用：OnError/OnClose → g.cleanupSession(session) →
sessionManager.releaseSession(session) → g.cleanupSessionResources(session) →
cleanupSession(session)。虽然幂等无害，但反映清理职责归属不清。建议清理逻辑只在一处执行，推荐仅在
SessionManager.releaseSession 内部执行清理。
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
