fix(style): code style and process overhead - #28
Merged
Merged
Conversation
Signed-off-by: Maximilian Hagen <git@saxy.dev>
Signed-off-by: Maximilian Hagen <git@saxy.dev>
Signed-off-by: Maximilian Hagen <git@saxy.dev>
…rocessing bycrpt Signed-off-by: Maximilian Hagen <git@saxy.dev>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR consolidates binary network codec and client operations through shared helpers. It also adds bounded asynchronous RESP authentication with pipeline gating, failure limits, worker lifecycle management, and integration coverage. ChangesNetwork protocol helpers
Asynchronous RESP authentication
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant RESPClient
participant RESPServer
participant authJobQueue
participant bcryptWorker
participant gnetConnection
RESPClient->>RESPServer: Send AUTH and pipelined command
RESPServer->>authJobQueue: Queue copied credentials
RESPServer-->>RESPClient: Pause pipelined command processing
authJobQueue->>bcryptWorker: Verify copied credentials
bcryptWorker->>gnetConnection: Wake with authentication result
gnetConnection->>RESPServer: Resume processing
RESPServer-->>RESPClient: Send AUTH and command responses
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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)
internal/resp/server.go (1)
193-211: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftDo not close
authJobswhile event-loop goroutines may still send to it.
ListenAndServeblocks ingnet.Run, andOnBootonly closess.readyafters.engis assigned. In thectx.Done()shutdown path, closes.authJobsonly afterOnBoothas run successfully ands.eng.Stop(ctx)is guaranteed not to return early. Use a single stop marker orsync.Oncechecked indispatchAuth/OnBoot, then close the channel only after shutdown is complete. Also makeShutdown(ctx)bounded byctxinstead of blocking indefinitely ons.workerWg.Wait().🤖 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 `@internal/resp/server.go` around lines 193 - 211, The Shutdown method can close authJobs while event-loop goroutines still send to it and can block indefinitely waiting for workers. Coordinate shutdown through a single stop marker or sync.Once shared by Shutdown, dispatchAuth, and OnBoot; only close authJobs after OnBoot has completed successfully and eng.Stop(ctx) has fully completed. Update worker waiting in Shutdown to honor ctx cancellation rather than unconditionally blocking on workerWg.Wait().
🧹 Nitpick comments (1)
internal/network/role.go (1)
231-237: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract
encodeU16Stringto match the existingdecodeU16Stringhelper.
The decode side already has a shared single-string helper (decodeU16String), but the encode side still repeats the "check length, append length, append bytes" pattern inline in three places across two files. Extracting a symmetricencodeU16String(buf []byte, s string) ([]byte, bool)removes the remaining duplication thatencodeStringList/encodeByteListdid not cover.
internal/network/role.go#L231-L237: replace the inlineNamelength-check-and-append block (lines 226-230) withencodeU16String.internal/network/acl.go#L58-L64: replace the inlineUsernameandRolelength-check-and-append blocks (lines 43-52) withencodeU16String.🤖 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 `@internal/network/role.go` around lines 231 - 237, The string encoding logic is duplicated instead of using a shared helper. Add encodeU16String(buf []byte, s string) ([]byte, bool) alongside the existing decodeU16String, then replace the inline Name block in internal/network/role.go lines 226-230 and the Username and Role blocks in internal/network/acl.go lines 43-52; the consolidated sites internal/network/role.go lines 231-237 and internal/network/acl.go lines 58-64 require these call-site updates.
🤖 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 `@internal/resp/server.go`:
- Around line 193-211: The Shutdown method can close authJobs while event-loop
goroutines still send to it and can block indefinitely waiting for workers.
Coordinate shutdown through a single stop marker or sync.Once shared by
Shutdown, dispatchAuth, and OnBoot; only close authJobs after OnBoot has
completed successfully and eng.Stop(ctx) has fully completed. Update worker
waiting in Shutdown to honor ctx cancellation rather than unconditionally
blocking on workerWg.Wait().
---
Nitpick comments:
In `@internal/network/role.go`:
- Around line 231-237: The string encoding logic is duplicated instead of using
a shared helper. Add encodeU16String(buf []byte, s string) ([]byte, bool)
alongside the existing decodeU16String, then replace the inline Name block in
internal/network/role.go lines 226-230 and the Username and Role blocks in
internal/network/acl.go lines 43-52; the consolidated sites
internal/network/role.go lines 231-237 and internal/network/acl.go lines 58-64
require these call-site updates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1a87def9-5eb4-4533-a47b-8dcd6b230a81
📒 Files selected for processing (9)
internal/network/acl.gointernal/network/client.gointernal/network/client_acl.gointernal/network/client_role.gointernal/network/role.gointernal/network/server.gointernal/resp/benchmark_test.gointernal/resp/server.gointernal/resp/server_test.go
Signed-off-by: Maximilian Hagen <git@saxy.dev>
…eString and encodeByte Signed-off-by: Maximilian Hagen <git@saxy.dev>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Component:
Network / General
Type of Change:
Related Issue
none
Technical Deep Dive & Context
Performance & Benchmarks (If Applicable)
none
How Has This Been Tested?
go testChecklist
go test ./...andgo test -race ./...)go vetwarningsSummary by CodeRabbit
New Features
Bug Fixes