Skip to content

fix: fix issue detect by doubao #94

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.

Sign up for GitHub

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

Open
Wangzy455 wants to merge 5 commits into
base: master
Choose a base branch
from
Open

fix: fix issue detect by doubao #94

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
787e2de
fix issue
Wangzy455 Sep 21, 2025
eb0c902
fix issue
Wangzy455 Sep 22, 2025
3f94800
fix issue
Wangzy455 Sep 23, 2025
6974818
fix issue
Wangzy455 Sep 26, 2025
f61e2cb
fix issue
Wangzy455 Sep 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions transport/callback.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ func (t *callbacks) Add(handler, key any, callback func()) {

// Guard: avoid runtime panic on non-comparable types
if !isComparable(handler) || !isComparable(key) {
log.Error(perrors.New(fmt.Sprintf("callbacks.Add: non-comparable handler/key: %T, %T; ignored", handler, key)))
_ = log.Error(perrors.New(fmt.Sprintf("callbacks.Add: non-comparable handler/key: %T, %T; ignored", handler, key)))
return
}

Expand Down Expand Up @@ -109,7 +109,7 @@ func (t *callbacks) Add(handler, key any, callback func()) {
func (t *callbacks) Remove(handler, key any) {
// Guard: avoid runtime panic on non-comparable types
if !isComparable(handler) || !isComparable(key) {
log.Error(perrors.New(fmt.Sprintf("callbacks.Remove: non-comparable handler/key: %T, %T; ignored", handler, key)))
_ = log.Error(perrors.New(fmt.Sprintf("callbacks.Remove: non-comparable handler/key: %T, %T; ignored", handler, key)))
return
}

Expand Down
2 changes: 1 addition & 1 deletion transport/connection.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,7 @@ func (t *gettyTCPConn) CloseConn(waitSec int) {
if t.conn != nil {
if writer, ok := t.writer.(*snappy.Writer); ok {
if err := writer.Close(); err != nil {
log.Errorf("snappy.Writer.Close() = error:%+v", err)
_ = log.Errorf("snappy.Writer.Close() = error:%+v", err)
}
}
if conn, ok := t.conn.(*net.TCPConn); ok {
Expand Down
6 changes: 3 additions & 3 deletions transport/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ func (s *server) stop() {
if err := s.server.Shutdown(ctx); err != nil {
// if the log output is "shutdown ctx: context deadline exceeded"， it means that
// there are still some active connections.
log.Errorf("server shutdown ctx:%s error:%v", ctx, err)
_ = log.Errorf("server shutdown ctx:%s error:%v", ctx, err)
}
cancel()
}
Expand Down Expand Up @@ -424,7 +424,7 @@ func (s *server) runWSEventLoop(newSession NewSessionCallback) {
s.lock.Unlock()
err = server.Serve(s.streamListener)
if err != nil {
log.Errorf("http.server.Serve(addr{%s}) = err:%+v", s.addr, perrors.WithStack(err))
_ = log.Errorf("http.server.Serve(addr{%s}) = err:%+v", s.addr, perrors.WithStack(err))
}
}()
}
Expand Down Expand Up @@ -484,7 +484,7 @@ func (s *server) runWSSEventLoop(newSession NewSessionCallback) {
s.lock.Unlock()
err = server.Serve(tls.NewListener(s.streamListener, config))
if err != nil {
log.Errorf("http.server.Serve(addr{%s}) = err:%+v", s.addr, perrors.WithStack(err))
_ = log.Errorf("http.server.Serve(addr{%s}) = err:%+v", s.addr, perrors.WithStack(err))
panic(err)
}
}()
Expand Down
30 changes: 20 additions & 10 deletions transport/session.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,9 @@
// callbacks
closeCallback callbacks
closeCallbackMutex sync.RWMutex

// wait
closeWait chan struct{}
}
Comment on lines +145 to 148

@coderabbitai coderabbitai Bot Sep 22, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

New closeWait channel isn’t reinitialized in Reset(); close(nil) panic risk.

handlePackage closes s.closeWait. After Reset(), closeWait becomes nil (zero value) and closing it will panic.

Apply this diff to Reset():

 func (s *session) Reset() {
     *s = session{
         name:   defaultSessionName,
         once:   &sync.Once{},
         done:   make(chan struct{}),
         period: period,
         wait:   pendingDuration,
-        attrs:  gxcontext.NewValuesContext(context.Background()),
+        attrs:  gxcontext.NewValuesContext(context.Background()),
+        closeWait: make(chan struct{}),
     }
 }

Also applies to: 161-166

🤖 Prompt for AI Agents 
In transport/session.go around lines 145-148 and also touching Reset() around
161-166, the closeWait channel is left nil after Reset(), causing a panic when
handlePackage later calls close(s.closeWait); modify Reset() to reinitialize the
channel (e.g. assign a new non-nil channel to s.closeWait) and update any close
calls to guard against nil (or use a safe close pattern such as checking for
non-nil or using sync.Once) so closing cannot panic after a Reset().

@AlexStocks AlexStocks Sep 27, 2025

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good advice


func newSession(endPoint EndPoint, conn Connection) *session {
Expand All @@ -155,10 +158,11 @@

period: period,

once: &sync.Once{},
done: make(chan struct{}),
wait: pendingDuration,
attrs: gxcontext.NewValuesContext(context.Background()),
once: &sync.Once{},
done: make(chan struct{}),
wait: pendingDuration,
attrs: gxcontext.NewValuesContext(context.Background()),
closeWait: make(chan struct{}),
}

ss.Connection.SetSession(ss)
Expand Down Expand Up @@ -393,7 +397,7 @@
rBuf := make([]byte, size)
rBuf = rBuf[:runtime.Stack(rBuf, false)]
err = perrors.WithStack(fmt.Errorf("[session.WritePkg] panic session %s: err=%v\n%s", s.sessionToken(), r, rBuf))
log.Error(err)
_ = log.Error(err)
}
}()

Expand Down Expand Up @@ -553,14 +557,14 @@
if s.Connection == nil || s.listener == nil || s.writer == nil {
errStr := fmt.Sprintf("session{name:%s, conn:%#v, listener:%#v, writer:%#v}",
s.name, s.Connection, s.listener, s.writer)
log.Error(errStr)
_ = log.Error(errStr)
panic(errStr)
}

// call session opened
s.UpdateActive()
if err := s.listener.OnOpen(s); err != nil {
log.Errorf("[OnOpen] session %s, error: %#v", s.Stat(), err)
_ = log.Errorf("[OnOpen] session %s, error: %#v", s.Stat(), err)
s.Close()
return
}
Expand All @@ -578,7 +582,7 @@
f := func() {
// If the session is closed, there is no need to perform CPU-intensive operations.
if s.IsClosed() {
log.Errorf("[Id:%d, name=%s, endpoint=%s] Session is closed", s.ID(), s.name, s.EndPoint())
_ = log.Errorf("[Id:%d, name=%s, endpoint=%s] Session is closed", s.ID(), s.name, s.EndPoint())
return
}
s.listener.OnMessage(s, pkg)
Expand All @@ -599,13 +603,16 @@
const size = 64 << 10
rBuf := make([]byte, size)
rBuf = rBuf[:runtime.Stack(rBuf, false)]
log.Errorf("[session.handlePackage] panic session %s: err=%s\n%s", s.sessionToken(), r, rBuf)

Check failure on line 606 in transport/session.go

View workflow job for this annotation

GitHub Actions / CI (1.25, ubuntu-latest) 

Error return value of `log.Errorf` is not checked (errcheck)
}
grNum := s.grNum.Add(-1)
log.Infof("%s, [session.handlePackage] gr will exit now, left gr num %d", s.sessionToken(), grNum)
if grNum == 0 {
close(s.closeWait)
}
s.stop()
if err != nil {
log.Errorf("%s, [session.handlePackage] error:%+v", s.sessionToken(), perrors.WithStack(err))

Check failure on line 615 in transport/session.go

View workflow job for this annotation

GitHub Actions / CI (1.25, ubuntu-latest) 

Error return value of `log.Errorf` is not checked (errcheck)
if s != nil || s.listener != nil {
s.listener.OnError(s, err)
}
Expand All @@ -618,7 +625,7 @@
if _, ok := s.Connection.(*gettyTCPConn); ok {
if s.reader == nil {
errStr := fmt.Sprintf("session{name:%s, conn:%#v, reader:%#v}", s.name, s.Connection, s.reader)
log.Error(errStr)
_ = log.Error(errStr)
panic(errStr)
}

Expand Down Expand Up @@ -658,7 +665,7 @@
ctx, cancel := context.WithTimeout(context.Background(), tlsHandshaketime)
defer cancel()
if err := tlsConn.HandshakeContext(ctx); err != nil {
log.Errorf("[tlsConn.HandshakeContext] = error:%+v", err)

Check failure on line 668 in transport/session.go

View workflow job for this annotation

GitHub Actions / CI (1.25, ubuntu-latest) 

Error return value of `log.Errorf` is not checked (errcheck)
return perrors.Wrap(err, "tlsConn.HandshakeContext")
}
}
Expand Down Expand Up @@ -877,7 +884,7 @@
rBuf = rBuf[:runtime.Stack(rBuf, false)]
err := perrors.WithStack(fmt.Errorf("[session.invokeCloseCallbacks] panic session %s: err=%v\n%s",
sessionToken, r, rBuf))
log.Error(err)
_ = log.Error(err)
}
}()

Expand Down Expand Up @@ -914,6 +921,9 @@
// Close will be invoked by NewSessionCallback(if return error is not nil)
// or (session)handleLoop automatically. It's thread safe.
func (s *session) Close() {
if s.IsClosed() {
return
}
s.stop()
log.Infof("%s closed now. its current gr num is %d", s.sessionToken(), s.grNum.Load())
}
Expand Down
33 changes: 24 additions & 9 deletions util/logger.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,29 @@ import (
type Logger interface {
Info(args ...any)
Warn(args ...any)
Error(args ...any)
Error(args ...any) error
Debug(args ...any)
Infof(fmt string, args ...any)
Warnf(fmt string, args ...any)
Errorf(fmt string, args ...any)
Errorf(fmt string, args ...any) error
Debugf(fmt string, args ...any)
}
Comment on lines +29 to 35

@coderabbitai coderabbitai Bot Sep 22, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

🧩 Analysis chain

Breaking change: Logger.Error/Errorf now return error. Provide a backward‑compat shim.

This breaks existing custom loggers passed via SetLogger. Add a LegacyLogger adapter to preserve compatibility.

Proposed additions (outside the hunks):

// LegacyLogger mirrors the old contract (no error returns).
type LegacyLogger interface {
    Info(args ...any)
    Warn(args ...any)
    Error(args ...any)
    Debug(args ...any)
    Infof(string, ...any)
    Warnf(string, ...any)
    Errorf(string, ...any)
    Debugf(string, ...any)
}

// Wrap a legacy logger so it satisfies the new Logger.
type legacyLoggerAdapter struct{ LegacyLogger }

func (l legacyLoggerAdapter) Error(args ...any) error  { l.LegacyLogger.Error(args...); return nil }
func (l legacyLoggerAdapter) Errorf(f string, a ...any) error {
    l.LegacyLogger.Errorf(f, a...); return nil
}

// SetLegacyLogger allows existing integrations to continue working.
func SetLegacyLogger(legacy LegacyLogger) { log = legacyLoggerAdapter{LegacyLogger: legacy} }

Search for external uses in this repo:

🏁 Script executed:

#!/bin/bash
rg -nP '\bSetLogger\(' -C2

Length of output: 183

Add LegacyLogger adapter for backward compatibility
Logger.Error and Errorf now return an error, which will break any existing custom loggers passed via SetLogger. Define a LegacyLogger interface and a legacyLoggerAdapter plus SetLegacyLogger shim to preserve the old (no-error) contract:

// LegacyLogger mirrors the old contract (no error returns).
type LegacyLogger interface {
    Info(args ...any)
    Warn(args ...any)
    Error(args ...any)
    Debug(args ...any)
    Infof(string, ...any)
    Warnf(string, ...any)
    Errorf(string, ...any)
    Debugf(string, ...any)
}

// legacyLoggerAdapter wraps a LegacyLogger to satisfy the new Logger.
type legacyLoggerAdapter struct{ LegacyLogger }

func (l legacyLoggerAdapter) Error(args ...any) error {
    l.LegacyLogger.Error(args...)
    return nil
}
func (l legacyLoggerAdapter) Errorf(f string, a ...any) error {
    l.LegacyLogger.Errorf(f, a...)
    return nil
}

// SetLegacyLogger installs a LegacyLogger for compatibility with SetLogger.
func SetLegacyLogger(legacy LegacyLogger) {
    log = legacyLoggerAdapter{LegacyLogger: legacy}
}
🤖 Prompt for AI Agents 
In util/logger.go around lines 29 to 35, add a LegacyLogger interface mirroring
the old no-error methods and implement a legacyLoggerAdapter that wraps a
LegacyLogger and implements the new Logger by delegating calls and returning nil
for Error and Errorf; then add a SetLegacyLogger(legacy LegacyLogger) shim that
assigns log = legacyLoggerAdapter{LegacyLogger: legacy} so existing custom
loggers continue to work without modifying their signatures.


// zapLoggerAdapter adapts zap.SugaredLogger to the Logger interface
type zapLoggerAdapter struct {
*zap.SugaredLogger
}

func (l *zapLoggerAdapter) Error(args ...any) error {
l.SugaredLogger.Error(args...)
return nil
}

func (l *zapLoggerAdapter) Errorf(fmt string, args ...any) error {
l.SugaredLogger.Errorf(fmt, args...)
return nil
}

type LoggerLevel int8

const (
Expand Down Expand Up @@ -79,7 +94,7 @@ var (
func init() {
zapLoggerConfig.EncoderConfig = zapLoggerEncoderConfig
zapLogger, _ = zapLoggerConfig.Build()
log = zapLogger.Sugar()
log = &zapLoggerAdapter{zapLogger.Sugar()}

// todo: flushes buffer when redirect log to file.
// var exitSignal = make(chan os.Signal)
Expand Down Expand Up @@ -114,7 +129,7 @@ func SetLoggerLevel(level LoggerLevel) error {
if err != nil {
return err
}
log = zapLogger.Sugar()
log = &zapLoggerAdapter{zapLogger.Sugar()}
return nil
}

Expand All @@ -128,7 +143,7 @@ func SetLoggerCallerDisable() error {
if err != nil {
return err
}
log = zapLogger.Sugar()
log = &zapLoggerAdapter{zapLogger.Sugar()}
return nil
}

Expand Down Expand Up @@ -163,11 +178,11 @@ func Warnf(template string, args ...any) {
}

// Error
func Error(args ...any) {
log.Error(args...)
func Error(args ...any) error {
return log.Error(args...)
}

// Errorf
func Errorf(template string, args ...any) {
log.Errorf(template, args...)
func Errorf(template string, args ...any) error {
return log.Errorf(template, args...)
}
Loading