revert: restore the RPC layer (pkg/rpc)#207
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds Goridge-backed ChangesRPC codec integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ClientCodec
participant Relay
participant ServerCodec
participant GoService
Client->>ClientCodec: Write RPC request
ClientCodec->>Relay: Send encoded Goridge frame
Relay->>ServerCodec: Receive request frame
ServerCodec->>GoService: Decode and dispatch method
GoService-->>ServerCodec: Return response
ServerCodec->>Relay: Send encoded response frame
Relay-->>ClientCodec: Receive response
ClientCodec-->>Client: Decode response body
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #207 +/- ##
===========================================
- Coverage 90.59% 71.37% -19.22%
===========================================
Files 5 8 +3
Lines 372 828 +456
===========================================
+ Hits 337 591 +254
- Misses 21 201 +180
- Partials 14 36 +22 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Reverts the prior removal of the pkg/rpc layer by restoring the Goridge-backed net/rpc ServerCodec/ClientCodec, reintroducing protobuf support used by RPC tests/benchmarks, and updating CI/docs to reflect the restored RPC workflow.
Changes:
- Re-adds
pkg/rpccodecs and their unit/integration tests. - Restores protobuf-based test fixtures and benchmark harnesses.
- Updates README, Makefile, and CI workflows to include
./pkg/rpctests and the protobuf dependency.
Reviewed changes
Copilot reviewed 16 out of 19 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/php_test_files/issue_185.php | PHP-side RPC call fixture used by an issue regression test. |
| tests/php_test_files/composer.json | Adds Composer requirement for the PHP Goridge package. |
| tests/php_test_files/composer.lock | Locks PHP dependencies for the PHP test fixtures. |
| tests/message.proto | Reintroduces protobuf schema used by Go RPC tests. |
| tests/message.pb.go | Generated Go types from tests/message.proto. |
| tests/issues/issue_185_test.go | Adds Go test exercising PHP↔Go RPC interoperability for Issue #185. |
| README.md | Reframes docs and sample usage around net/rpc + Goridge codec. |
| pkg/rpc/doc.go | Package documentation for the restored RPC layer. |
| pkg/rpc/codec.go | Restored server-side net/rpc codec implementation. |
| pkg/rpc/codec_edge_test.go | Edge/regression tests for codec safety and close behavior. |
| pkg/rpc/client.go | Restored client-side net/rpc codec implementation. |
| pkg/rpc/client_server_test.go | End-to-end codec tests across proto/json/raw/error/concurrency. |
| Makefile | Adds go test invocation for ./pkg/rpc. |
| go.mod | Restores google.golang.org/protobuf dependency. |
| go.sum | Updates checksums for restored Go deps. |
| benchmarks/main.go | Restores benchmark harness for RPC codec and pprof server. |
| .github/workflows/linux.yml | Adds ./pkg/rpc to Linux CI test matrix/coverage. |
| .github/workflows/macos.yml | Adds ./pkg/rpc to macOS CI test matrix/coverage. |
| .github/workflows/windows.yml | Adds ./pkg/rpc to Windows CI test matrix. |
Files not reviewed (1)
- tests/message.pb.go: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
benchmarks/main.go (1)
122-124: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueSet timeouts on
http.Server.The
http.Serverfor debugging is constructed without timeouts (e.g.,ReadHeaderTimeout,ReadTimeout). Even though this is a local benchmark server, setting timeouts is a good practice to prevent resource exhaustion and connection leaks.♻️ Proposed refactor
- return Server{srv: &http.Server{Handler: mux}} + return Server{srv: &http.Server{ + Handler: mux, + ReadHeaderTimeout: 5 * time.Second, + ReadTimeout: 10 * time.Second, + }}🤖 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 `@benchmarks/main.go` around lines 122 - 124, Update the http.Server construction in the Server initializer to configure appropriate request timeouts, including ReadHeaderTimeout and ReadTimeout, while preserving the existing Handler mux assignment.Source: Linters/SAST tools
README.md (1)
72-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unnecessary blank identifier assignment.
The
_ = connassignment is unnecessary sinceconnis already used in the followingrpc.ServeCodeccall.♻️ Proposed refactor
for { conn, err := ln.Accept() if err != nil { continue } - _ = conn go rpc.ServeCodec(goridgeRpc.NewCodec(conn)) }🤖 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 `@README.md` around lines 72 - 79, Remove the unnecessary `_ = conn` assignment from the listener loop; retain the existing `conn` usage in the subsequent `rpc.ServeCodec` call.pkg/rpc/codec.go (1)
89-89: 📐 Maintainability & Code Quality | 🔵 TrivialRepeated
codec.(byte)assertions.
codec.(byte)is re-asserted independently at eachcaseafter thecodec == nilguard. Extracting it once into a localbytevariable after Line 100 would avoid the repetition without behavior change.Also applies to: 103-103, 124-124, 154-154, 176-176, 179-179
🤖 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 `@pkg/rpc/codec.go` at line 89, In the codec handling function, extract the guarded codec value from the repeated codec.(byte) assertions into one local byte variable after the codec == nil check, then reuse that variable in the affected WriteFlags and related case branches without changing behavior.pkg/rpc/client.go (1)
116-140: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueFrame not returned to pool on early-return error paths.
None of the four error returns in
ReadResponseHeader(Receive failure, CRC failure, bad options count, offset-out-of-bounds) callc.putFrame/release the fetched frame, unlikecodec.go'sReadRequestHeader, which explicitly frees the frame before every error return. Impact is muted here becausenet/rpc.Client.input()closes the whole client on a header-read error, but it's worth aligning for consistency and to avoid holding onto the pool object longer than necessary.🤖 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 `@pkg/rpc/client.go` around lines 116 - 140, Update ClientCodec.ReadResponseHeader to return the frame obtained by getFrame through c.putFrame before each error return: Receive failure, CRC verification failure, invalid options count, and payload offset out of bounds. Keep the successful path assigning c.frame unchanged, matching ReadRequestHeader’s frame-release behavior.
🤖 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.
Inline comments:
In `@pkg/rpc/codec_edge_test.go`:
- Around line 95-104: Update ReadResponseBody to guard c.frame before the
deferred putFrame reset, so calling ReadResponseBody(nil) before a successful
ReadResponseHeader cannot panic. Preserve the existing frame cleanup behavior
when c.frame is non-nil.
In `@pkg/rpc/codec.go`:
- Around line 282-289: Move the defer registering c.putFrame(c.frame) in
Codec.ReadRequestBody before the out == nil check, preserving the existing nil
return while ensuring the frame is returned to the pool for unresolved or
malformed requests.
- Around line 102-108: Update the Proto branch in the response encoding logic
around proto.Marshal to use a safe two-value assertion for body as
proto.Message. On mismatch, return through c.handleError with an appropriate
error instead of panicking, while preserving the existing marshal-error handling
for valid protobuf replies.
In `@tests/issues/issue_185_test.go`:
- Around line 34-42: Update the goroutine in the issue_185 test to create a
context with a finite timeout and pass it to exec.CommandContext instead of
context.Background(). Ensure the context is canceled after use, while preserving
the existing PHP command and assertions.
In `@tests/message.proto`:
- Line 4: Update the go_package option in message.proto so its package-name
segment is tests, matching the generated code and existing tests.* callers;
preserve the current local import path segment.
In `@tests/php_test_files/issue_185.php`:
- Around line 3-4: Update the autoload require in the PHP test script to resolve
vendor/autoload.php relative to the script’s own directory rather than the
process working directory, preserving the existing Composer vendor location used
by issue_185_test.go.
---
Nitpick comments:
In `@benchmarks/main.go`:
- Around line 122-124: Update the http.Server construction in the Server
initializer to configure appropriate request timeouts, including
ReadHeaderTimeout and ReadTimeout, while preserving the existing Handler mux
assignment.
In `@pkg/rpc/client.go`:
- Around line 116-140: Update ClientCodec.ReadResponseHeader to return the frame
obtained by getFrame through c.putFrame before each error return: Receive
failure, CRC verification failure, invalid options count, and payload offset out
of bounds. Keep the successful path assigning c.frame unchanged, matching
ReadRequestHeader’s frame-release behavior.
In `@pkg/rpc/codec.go`:
- Line 89: In the codec handling function, extract the guarded codec value from
the repeated codec.(byte) assertions into one local byte variable after the
codec == nil check, then reuse that variable in the affected WriteFlags and
related case branches without changing behavior.
In `@README.md`:
- Around line 72-79: Remove the unnecessary `_ = conn` assignment from the
listener loop; retain the existing `conn` usage in the subsequent
`rpc.ServeCodec` call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 804be455-edc7-4502-8ea9-0a0092e617d8
⛔ Files ignored due to path filters (3)
go.sumis excluded by!**/*.sumtests/message.pb.gois excluded by!**/*.pb.gotests/php_test_files/composer.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
.github/workflows/linux.yml.github/workflows/macos.yml.github/workflows/windows.ymlMakefileREADME.mdbenchmarks/main.gogo.modpkg/rpc/client.gopkg/rpc/client_server_test.gopkg/rpc/codec.gopkg/rpc/codec_edge_test.gopkg/rpc/doc.gotests/issues/issue_185_test.gotests/message.prototests/php_test_files/composer.jsontests/php_test_files/issue_185.php
Use the two-value type assertion for the reply body in the CodecProto branch and return an error response instead of an unrecovered panic when the body is not a proto.Message, matching the safe pattern already used in ReadRequestBody/ReadResponseBody. Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
…hRelay - ReadRequestBody: return the frame stored by ReadRequestHeader when net/rpc drains an unresolvable request with a nil out, instead of leaking it from the pool (guard the nil frame to keep the direct-call path panic-free). - NewCodecWithRelay: initialize bPool/fPool so get()/getFrame() no longer panic on a zero-value sync.Pool. - tests: resolve issue_185.php autoload relative to __DIR__ and align message.proto go_package name with the generated `tests` package. Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
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
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.
Reverts #203. Connect-RPC is being reverted across the org: the rpc plugin serves goridge/net-rpc again, so the codec, client and their tests return. Also restores the protobuf dependency and keeps errors at v1.5.0.
Summary by CodeRabbit
net/rpccodec support for Proto, JSON, Gob, and raw byte payloads.rpcpackage documentation.testtarget to run additional./pkg/rpctests with coverage output.