Conversation
WalkthroughLogger.Error/Errorf now return error and a zap adapter was added; many logging call sites were updated to explicitly discard the returned error (assign to Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Session
participant Handler
participant closeWait as closeWait (chan)
Note over Session: Session now owns `closeWait`
Client->>Session: start handler (increment grNum)
Session->>Handler: run handler goroutine
Handler-->>Session: handler done (decrement grNum)
alt grNum > 0
Note right of Session: other handlers still running
else grNum == 0
Session->>closeWait: close(closeWait)
Note right of closeWait: handlers drained — signal released
end
Client->>Session: Close()
Session->>Session: if IsClosed() return early
Note over Session: Close may wait on `closeWait` before final cleanup
Note over Session: logging calls now use `_ = log.Error(...)` to discard adapter errors
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (6)
🧰 Additional context used🧬 Code graph analysis (6)examples/echo/tcp-echo/client/app/handler.go (2)
examples/echo/tcp-echo/client/app/echo.go (1)
examples/echo/tcp-echo/server/app/readwriter.go (1)
examples/echo/tcp-echo/server/app/handler.go (1)
examples/echo/tcp-echo/client/app/readwriter.go (1)
examples/echo/tcp-echo/server/app/echo.go (1)
🔇 Additional comments (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
transport/connection.go (1)
340-345: Also close flate writer to flush trailing bytes.We only close the snappy writer. If
SetCompressTypeselected flate,writeFlusher.flusher.Close()should be called to flush and free resources.Apply this diff:
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) } + } else if wf, ok := t.writer.(*writeFlusher); ok && wf.flusher != nil { + if err := wf.flusher.Close(); err != nil { + _ = log.Errorf("flate.Writer.Close() = error:%+v", err) + } }transport/callback.go (2)
72-76: Logging style nit: avoid constructing an error just to log.Prefer formatting directly via
Errorfto skip an allocation.- _ = log.Error(perrors.New(fmt.Sprintf("callbacks.Add: non-comparable handler/key: %T, %T; ignored", handler, key))) + _ = log.Errorf("callbacks.Add: non-comparable handler/key: %T, %T; ignored", handler, key)
110-114: Same here for Remove().Mirror the style from Add().
- _ = log.Error(perrors.New(fmt.Sprintf("callbacks.Remove: non-comparable handler/key: %T, %T; ignored", handler, key))) + _ = log.Errorf("callbacks.Remove: non-comparable handler/key: %T, %T; ignored", handler, key)transport/server.go (1)
425-428: Don’t log expected http.ErrServerClosed on normal shutdown.
Servereturnshttp.ErrServerClosedafterShutdown. Filter it to reduce noise.- err = server.Serve(s.streamListener) - if err != nil { - _ = log.Errorf("http.server.Serve(addr{%s}) = err:%+v", s.addr, perrors.WithStack(err)) - } + if err = server.Serve(s.streamListener); err != nil && !errors.Is(err, http.ErrServerClosed) { + _ = log.Errorf("http.server.Serve(addr{%s}) = err:%+v", s.addr, perrors.WithStack(err)) + }Add import:
import ( "context" "crypto/tls" "crypto/x509" "fmt" "net" "net/http" "os" "strings" "sync" "time" + "errors" )transport/session.go (2)
610-612: Close closeWait only when non-nil (belt-and-suspenders).Even after fixing Reset(), guarding the close is safer during refactors.
- if grNum == 0 { - close(s.closeWait) - } + if grNum == 0 && s.closeWait != nil { + close(s.closeWait) + }
145-148: Expose a wait helper to make closeWait useful.Optional: add
WaitClosed(ctx)so callers can block until goroutines drain.Add method:
// WaitClosed blocks until the session’s worker goroutine exits or ctx times out. func (s *session) WaitClosed(ctx context.Context) error { if s == nil { return nil } select { case <-s.closeWait: return nil case <-ctx.Done(): return ctx.Err() } }Would you like me to wire this into callers (e.g., server shutdown) if present?
util/logger.go (1)
181-188: Harden Error/Errorf against panics from custom loggers.Guard these wrappers with
recover()so logging can’t crash the process (one of the doubao findings).-// Error -func Error(args ...any) error { - return log.Error(args...) -} +// Error +func Error(args ...any) (err error) { + defer func() { + if r := recover(); r != nil { + err = fmt.Errorf("logger panic: %v", r) + } + }() + return log.Error(args...) +} -// Errorf -func Errorf(template string, args ...any) error { - return log.Errorf(template, args...) -} +// Errorf +func Errorf(template string, args ...any) (err error) { + defer func() { + if r := recover(); r != nil { + err = fmt.Errorf("logger panic: %v", r) + } + }() + return log.Errorf(template, args...) +}Add import:
import ( "go.uber.org/zap" "go.uber.org/zap/zapcore" + "fmt" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
transport/callback.go(2 hunks)transport/connection.go(1 hunks)transport/server.go(2 hunks)transport/session.go(5 hunks)util/logger.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
transport/connection.go (1)
util/logger.go (1)
Errorf(186-188)
transport/callback.go (1)
util/logger.go (1)
Error(181-183)
transport/server.go (1)
util/logger.go (1)
Errorf(186-188)
transport/session.go (1)
util/logger.go (1)
Error(181-183)
🔇 Additional comments (5)
transport/connection.go (1)
343-343: No-op assignment is fine; close error is logged.Using
_ = log.Errorf(...)is consistent with the new logger signature and keeps behavior unchanged.transport/server.go (1)
156-160: OK to discard logging error; consider severity.Keeping the error log on shutdown failure is good. No functional change introduced here.
transport/session.go (2)
401-401: Discarding the logger’s returned error is fine.The panic path is preserved; logging remains non-blocking.
924-926: Early return in Close() avoids redundant work.Good small guard;
stop()remains idempotent viaonce.Doanyway.util/logger.go (1)
37-50: Adapter approach looks good.Embedding
SugaredLoggerplus explicitError/Errorfsatisfies the interface cleanly.Also applies to: 97-98, 132-133, 146-147
|
|
||
| // wait | ||
| closeWait chan struct{} | ||
| } |
There was a problem hiding this comment.
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().
| 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) | ||
| } |
There was a problem hiding this comment.
🛠️ 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\(' -C2Length 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.
|
the ci failed. what a pity. |
Wangzy455
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
transport/session.go(13 hunks)transport/tls.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- transport/session.go
🧰 Additional context used
🧬 Code graph analysis (1)
transport/tls.go (1)
util/logger.go (2)
Error(181-183)Errorf(186-188)
| if ok := certPool.AppendCertsFromPEM(certPem); !ok { | ||
| log.Error("failed to parse root certificate file") | ||
| _ = log.Error("failed to parse root certificate file") | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Fix returning nil error on server cert parse failure
When AppendCertsFromPEM fails, err is still nil, so we return (nil, nil) and callers treat the TLS build as successful until they dereference a nil config. Please create and propagate a real error before returning.
- if ok := certPool.AppendCertsFromPEM(certPem); !ok {
- _ = log.Error("failed to parse root certificate file")
- return nil, err
+ if ok := certPool.AppendCertsFromPEM(certPem); !ok {
+ err = perrors.New("failed to parse root certificate file")
+ _ = log.Error(err)
+ return nil, err📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ok := certPool.AppendCertsFromPEM(certPem); !ok { | |
| log.Error("failed to parse root certificate file") | |
| _ = log.Error("failed to parse root certificate file") | |
| return nil, err | |
| } | |
| if ok := certPool.AppendCertsFromPEM(certPem); !ok { | |
| err = perrors.New("failed to parse root certificate file") | |
| _ = log.Error(err) | |
| return nil, err | |
| } |
🤖 Prompt for AI Agents
In transport/tls.go around lines 75 to 78, the code logs a failure from
certPool.AppendCertsFromPEM but returns the nil err variable, causing callers to
receive (nil, nil); create a real error (e.g., fmt.Errorf or errors.New)
describing the root certificate parse failure, log that error, and return (nil,
err) so the caller receives a non-nil error indicating TLS config construction
failed.
| if !ok { | ||
| log.Error("failed to parse root certificate") | ||
| _ = log.Error("failed to parse root certificate") | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Propagate a real error when client cert parsing fails
Same issue here: err is nil when AppendCertsFromPEM returns false, so the function hands back (nil, nil) and masks the failure. Please instantiate and return a proper error.
- ok := clientCertPool.AppendCertsFromPEM(certBytes)
- if !ok {
- _ = log.Error("failed to parse root certificate")
- return nil, err
+ ok := clientCertPool.AppendCertsFromPEM(certBytes)
+ if !ok {
+ err = perrors.New("failed to parse root certificate")
+ _ = log.Error(err)
+ return nil, err🤖 Prompt for AI Agents
In transport/tls.go around lines 108 to 111, when AppendCertsFromPEM returns
false the code currently logs an error but returns (nil, err) where err is nil,
causing a masked success; change this to create a real error (e.g. using
fmt.Errorf or errors.New with a clear message like "failed to parse root
certificate: AppendCertsFromPEM returned false"), log that error if desired, and
return (nil, thatError) so the caller receives a non-nil error.
2 participants
What this PR does:
fix issue93
Which issue(s) this PR fixes:
for #93
Special notes for your reviewer:
NONE
Does this PR introduce a user-facing change?:
NONE
Summary by CodeRabbit
Bug Fixes
Refactor
Stability