fix: fix issue detect by doubao
What this PR does: fix issue93
Which issue(s) this PR fixes:
for https://github.com/AlexStocks/getty/issues/93
Special notes for your reviewer: NONE
Does this PR introduce a user-facing change?: NONE
Summary by CodeRabbit
-
Bug Fixes
- Reduced spurious error output during shutdown and connection closure.
- Added session close coordination to ensure cleaner, reliable teardowns.
-
Refactor
- Logging API now returns errors; internal logging adapter and callers updated.
-
Stability
- Suppressed noisy logging side-effects to avoid misleading error traces during normal operation.
Walkthrough
Logger.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 _). Session gained a closeWait channel and an early Close guard; server shutdown/serve error paths now discard returned errors. Only Logger interface signatures were exportedly changed.
Changes
| Cohort / File(s) | Summary of Changes |
|---|---|
Logger API & adapterutil/logger.go |
Logger.Error and Logger.Errorf signatures changed to return error; added zapLoggerAdapter to wrap zap.SugaredLogger; global log init and public Error/Errorf functions updated to return/propagate adapter errors. |
Transport: logging-result discardtransport/callback.go, transport/connection.go, transport/server.go, transport/tls.go |
Replaced prior ignored log.Error/log.Errorf calls with explicit discards (_ = log.Error(...) / _ = log.Errorf(...)) across multiple error paths (callback add/remove, connection CloseConn/snappy close, server shutdown/serve, TLS build errors). No control-flow changes besides swallowing the logger-returned error. |
Session sync & guardstransport/session.go |
Added closeWait chan struct{} to session and initialize it; close closeWait when handler goroutine count (grNum) hits zero; added early-return guard in Close() if already closed; updated some panic/error logging calls to discard returned errors. |
Examples: echo handlers/readwritersexamples/echo/tcp-echo/server/..., examples/echo/tcp-echo/client/... |
Updated various example code logging calls to assign logging returns to _ (e.g., EchoPackage.Unmarshal, handlers, readwriter). Behavior unchanged aside from explicit discard of logger return values. |
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
- AlexStocks/getty#89 — touches
util/logger.goand Logger Error/Errorf API; likely related to the logging interface changes and adapter.
Suggested reviewers
- AlexStocks
Poem
A rabbit hops through lines of code,
I hush the logger's tiny cry.
A channel waits until it's closed,
Goroutines finish — then goodbye.
Blank IDs tuck errors away, so quiet I spy. 🐇✨
Pre-merge checks and finishing touches
❌ Failed checks (2 warnings)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Title Check | ⚠️ Warning | The title “fix: fix issue detect by doubao” does not clearly summarize the substantive change but merely indicates an unspecified fix referencing the reporter, offering no insight into what was altered or improved in the codebase. | Rename the title to concisely describe the main code change (for example “logger: discard unused return values to silence error warnings”) and remove the reference to the issue reporter. |
| Linked Issues Check | ⚠️ Warning | Although the changes standardize logging calls and introduce a session close notification channel, they only address a subset of the linked issue’s objectives around logging and goroutine coordination. The pull request does not implement atomic safety for session counters, RPC boundary markers, front-end XSS sanitization, dependency cleanup, Go version constraints, template improvements, or error feedback for one-way RPC calls. | Implement the remaining objectives or decompose the work into focused pull requests that each fully address a specific set of issues documented in #93. |
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Out of Scope Changes Check | ✅ Passed | All modifications in this pull request relate directly to enhancing logging behavior and managing session shutdown signaling, which correspond to the logging and concurrency objectives in the linked issue. There are no detected changes unrelated to those scopes. |
| Docstring Coverage | ✅ Passed | No functions found in the changes. Docstring coverage check skipped. |
✨ Finishing touches
- [ ] 📝 Generate Docstrings
🧪 Generate unit tests
- [ ] Create PR with unit tests
- [ ] Post copyable unit tests in a comment
📜 Recent review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 6974818002898f49260762cf09a50eb4859af2b3 and f61e2cbd11b16a031984a8df571cf6716c7bdf35.
📒 Files selected for processing (6)
examples/echo/tcp-echo/client/app/echo.go(1 hunks)examples/echo/tcp-echo/client/app/handler.go(2 hunks)examples/echo/tcp-echo/client/app/readwriter.go(1 hunks)examples/echo/tcp-echo/server/app/echo.go(1 hunks)examples/echo/tcp-echo/server/app/handler.go(1 hunks)examples/echo/tcp-echo/server/app/readwriter.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
examples/echo/tcp-echo/client/app/handler.go (2)
util/logger.go (1)
Error(181-183)examples/echo/ws-echo/client/app/handler.go (2)
h(66-75)h(77-89)
examples/echo/tcp-echo/client/app/echo.go (1)
util/logger.go (1)
Error(181-183)
examples/echo/tcp-echo/server/app/readwriter.go (1)
util/logger.go (1)
Error(181-183)
examples/echo/tcp-echo/server/app/handler.go (1)
util/logger.go (1)
Error(181-183)
examples/echo/tcp-echo/client/app/readwriter.go (1)
util/logger.go (1)
Error(181-183)
examples/echo/tcp-echo/server/app/echo.go (1)
util/logger.go (1)
Error(181-183)
🔇 Additional comments (6)
examples/echo/tcp-echo/server/app/echo.go (1)
135-135: Logging error return safely discardedAssigning the result to
_keeps the log side effect while appeasing the new error-returning signature—looks good.examples/echo/tcp-echo/client/app/echo.go (1)
135-135: Consistent handling of Logger.Error returnThis mirrors the updated logging API contract without altering behavior—no further changes needed.
examples/echo/tcp-echo/client/app/handler.go (1)
69-81: Good: Logger.Error results consistently ignoredBoth call sites now discard the returned error explicitly, matching the revised logger interface while preserving control flow.
examples/echo/tcp-echo/client/app/readwriter.go (1)
71-71: Acknowledged: no-op assignment for log.ErrorThe blank identifier keeps the log call and satisfies the new signature—LGTM.
examples/echo/tcp-echo/server/app/readwriter.go (1)
71-71: Logging change aligns with new signatureCapturing the return value with
_avoids unused-result noise while keeping the log; all good here.examples/echo/tcp-echo/server/app/handler.go (1)
136-143: Discarding the logging error is fine here.
log.Errornow surfaces anerror, and explicitly assigning to_keeps the original control flow intact without triggering unused-result warnings. No further action needed.
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 help to get the list of available commands and usage tips.
the ci failed. what a pity.
Please handle the CI