getty icon indicating copy to clipboard operation
getty copied to clipboard

fix: fix issue detect by doubao

Open Wangzy455 opened this issue 3 months ago • 3 comments

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.

Wangzy455 avatar Sep 21 '25 11:09 Wangzy455

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 & adapter
util/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 discard
transport/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 & guards
transport/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/readwriters
examples/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.go and 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 discarded

Assigning 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 return

This 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 ignored

Both 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.Error

The 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 signature

Capturing 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.Error now surfaces an error, 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot] avatar Sep 22 '25 14:09 coderabbitai[bot]

the ci failed. what a pity.

AlexStocks avatar Sep 23 '25 01:09 AlexStocks

Please handle the CI

LucienShen-Liu avatar Dec 06 '25 05:12 LucienShen-Liu