chore: simplify, modernize (Go 1.26), update deps#116
Conversation
- Drop explicit string type from pluginName/RrMode consts - Replace unsafe strToBytes with []byte(s) in config.go - Reset Codec/Flags in putPayload to avoid pool contamination - Remove redundant *staticPool.Pool type-assert in Stop; call Destroy via Pool interface - Defer RUnlock in Exec; eliminate scattered manual RUnlock calls - Use single defer putPayload in Handler.Start - Log discarded sendClose wPool error instead of silencing it - Clarify events.go comment: []byte vars are mutable globals, not Go consts - Document Release() as a no-op placeholder
|
Warning Review limit reached
More reviews will be available in 52 minutes and 11 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR removes the ChangesResource Management and Cleanup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #116 +/- ##
==========================================
+ Coverage 62.57% 63.05% +0.48%
==========================================
Files 4 4
Lines 171 157 -14
==========================================
- Hits 107 99 -8
+ Misses 52 47 -5
+ Partials 12 11 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR simplifies several control-flow and cleanup paths in the TCP plugin/handler, removes an unsafe string→bytes conversion in config initialization, and makes pooled payload reuse safer by resetting additional fields.
Changes:
- Simplify lock/unlock and cleanup patterns (e.g.,
deferforRUnlock, remove redundant pool type-assertion inStop). - Remove
unsafe-based string-to-bytes conversion in config delimiter initialization. - Harden payload pooling by resetting additional
payload.Payloadfields on return to the pool.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
plugin.go |
Simplifies constants, Stop() pool teardown, and Exec() lock handling. |
handler/pool.go |
Resets additional payload.Payload fields before returning to the pool. |
handler/helpers.go |
Ensures payload is returned to pool via defer and logs close-event send failures. |
handler/handler.go |
Simplifies connected-event payload cleanup and clarifies Release() as a no-op placeholder. |
handler/events.go |
Replaces misleading comment about []byte “immutability” with accurate explanation. |
config.go |
Removes unsafe helper and uses []byte(string) for delimiter bytes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…l call The payload used for the EventConnected call was deferred until Start() returned, keeping it checked out across the entire readLoop() lifetime and defeating pool reuse for long-lived connections.
NewPool returns a concrete *static_pool.Pool that is nil on error. Assigning it straight into the Pool interface field boxed a typed nil, so the p.wPool != nil guard in Stop passed and Destroy dereferenced a nil receiver. Assign the interface only after a successful NewPool.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugin.go (1)
127-149:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid blocking listener startup goroutines on
errCh.Line 128 allocates
errChwith capacity 1, but every listener goroutine in Lines 143-148 can send into it. If two listeners fail before the caller drains the first error, the later goroutine blocks forever onerrCh <- err.💡 Suggested fix
func (p *Plugin) Serve() chan error { - errCh := make(chan error, 1) + errCh := make(chan error, len(p.cfg.Servers)+1) + sendErr := func(err error) { + select { + case errCh <- err: + default: + p.log.Error("failed to report serve error", "error", err) + } + } // NewPool returns a concrete *static_pool.Pool; on error it is a nil // pointer. Assigning it directly to the Pool interface field would produce // a non-nil interface wrapping a nil pointer, so Stop's `p.wPool != nil` // guard would pass and Destroy would panic on a nil receiver. Assign the // interface only after a successful NewPool. wp, err := p.server.NewPool(context.Background(), p.cfg.Pool, map[string]string{RrMode: pluginName}, nil) if err != nil { - errCh <- err + sendErr(err) return errCh } p.wPool = wp @@ l, err := tcplisten.CreateListener(addr) if err != nil { - errCh <- err + sendErr(err) return }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugin.go` around lines 127 - 149, The listener goroutines can block on sending errors to errCh because it's created with capacity 1; change the creation of errCh in Serve to be buffered to accommodate all potential listener errors (e.g., errCh := make(chan error, len(p.cfg.Servers)+1) or similar), so multiple tcplisten.CreateListener failures from the goroutines won't block; keep the rest of the logic (sends to errCh in the goroutines) unchanged and ensure p.wPool assignment and error handling around p.server.NewPool remain as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@plugin.go`:
- Around line 127-149: The listener goroutines can block on sending errors to
errCh because it's created with capacity 1; change the creation of errCh in
Serve to be buffered to accommodate all potential listener errors (e.g., errCh
:= make(chan error, len(p.cfg.Servers)+1) or similar), so multiple
tcplisten.CreateListener failures from the goroutines won't block; keep the rest
of the logic (sends to errCh in the goroutines) unchanged and ensure p.wPool
assignment and error handling around p.server.NewPool remain as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7cab6af1-b4d4-4a89-9cd4-04b07dd95dd7
📒 Files selected for processing (6)
config.gohandler/events.gohandler/handler.gohandler/helpers.gohandler/pool.goplugin.go
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
Fixed (applied)
Simplify (S) — 5 findings:
handler/events.go: clarify comment —[]bytevars are package-level mutable globals, not Go consts; replaced misleading// immutablewith accurate explanationhandler/handler.go: singledefer putPayload(pld)inStart()replaces two manual calls in separate brancheshandler/helpers.go: log the discardedwPoolerror insendCloseinstead of silencing it with_, _handler/handler.go: documentRelease()as a no-op placeholder (was bare comment)plugin.go: removed redundant*staticPool.Pooltype-assert inStop;Destroyis already on thePoolinterfaceModernize (M) — 3 findings:
plugin.go: drop explicitstringtype onpluginNameandRrModeconstsconfig.go: replaceunsafe.Slice(unsafe.StringData(data), len(data))with plain[]byte(data); dropunsafeimporthandler/pool.go: resetCodecandFlagsfields inputPayloadto prevent pool contaminationReview/Low (R) — 1 finding:
plugin.go: defermu.RUnlock()inExec; eliminate four scattered manualRUnlockcalls that make future edits fragileNot applied (needs manual review)
handler/handler.go:103: doublesendClose()on partial-EOF path — subtle stream handling; needs test coveragehandler/handler.go:131: delimiter check only on current chunk — framing breaks across reads; requires protocol-level refactorplugin.go:172:Stopgoroutine holdsmuacrossDestroy; context-cancel → potential deadlock withResetplugin.go:128:errChcap 1 with multiple listeners — second failure blocks and leaks a goroutineVerification
go build ./...,go vet ./...,golangci-lint run— all clean.Summary by CodeRabbit
Release Notes
Bug Fixes
Documentation
Chores