fail any rpc call which blocks the runServer loop for more than 1s #1861
Conversation
… can't clear itself for 1s (an eternity) then fail the rpc to unblock the main loop
WalkthroughThe pull request introduces a series of modifications across multiple files in the project, focusing on enhancing the RPC (Remote Procedure Call) system and adding new functionality. The changes primarily involve updating function signatures to include additional parameters, such as debug names and client identifiers, across various utility and client-related files. A new ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
go.mod (1)
Line range hint
3-3: Fix invalid Go version.The specified Go version
1.23.4is invalid as Go versions only go up to 1.22.x currently. This will prevent builds from working correctly.Apply this diff to fix the Go version:
-go 1.23.4 +go 1.22.1
🧹 Nitpick comments (5)
pkg/util/ds/expmap.go (3)
15-19: Use a read-write mutex or separate locks for heap vs. map for potential high concurrency scenarios.
While a singlesync.Mutexis simpler, if the map will be accessed heavily in parallel, consider a read-write mutex or separate locks for the heap and map storage for improved concurrency performance.
42-48: Add optional background cleanup scheduler.
The current approach only expires items on demand (duringGet). Consider adding a background goroutine or scheduling mechanism to remove expired entries proactively, preventing the map from growing too large.
50-58: Revisit duplicate entries scenario.
Inserting new entries in the heap without removing old ones may lead to unnecessary memory usage. Maintaining a secondary index or removing duplicates could be beneficial in memory-constrained environments.cmd/wsh/cmd/wshcmd-test.go (1)
10-16: Extend usage documentation.
This hidden command may need more explanation or a “Long” description for users who discover it. Even hidden dev commands benefit from clarity on their purpose.pkg/wshrpc/wshremote/wshremote.go (1)
48-62: Consider enhancing the test command.While the
StreamTestCommandis useful for testing, consider:
- Adding configurable delay between messages to simulate slow processing
- Adding configurable message count and buffer size
- Adding documentation about its testing purpose
This would make it more versatile for testing RPC blocking scenarios.
func (impl *ServerImpl) StreamTestCommand(ctx context.Context) chan wshrpc.RespOrErrorUnion[int] { - ch := make(chan wshrpc.RespOrErrorUnion[int], 16) + const ( + defaultBufferSize = 16 + defaultCount = 1000 + defaultDelay = 0 * time.Millisecond + ) + ch := make(chan wshrpc.RespOrErrorUnion[int], defaultBufferSize) go func() { defer close(ch) idx := 0 for { + select { + case <-ctx.Done(): + return + default: + } ch <- wshrpc.RespOrErrorUnion[int]{Response: idx} idx++ - if idx == 1000 { + if idx == defaultCount { break } + time.Sleep(defaultDelay) } }() return ch }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
cmd/server/main-server.go(1 hunks)cmd/wsh/cmd/wshcmd-connserver.go(1 hunks)cmd/wsh/cmd/wshcmd-root.go(3 hunks)cmd/wsh/cmd/wshcmd-test.go(1 hunks)go.mod(1 hunks)pkg/util/ds/expmap.go(1 hunks)pkg/waveapp/waveapp.go(1 hunks)pkg/wshrpc/wshclient/barerpcclient.go(1 hunks)pkg/wshrpc/wshremote/wshremote.go(1 hunks)pkg/wshrpc/wshserver/wshserverutil.go(1 hunks)pkg/wshutil/wshrouter.go(0 hunks)pkg/wshutil/wshrpc.go(9 hunks)pkg/wshutil/wshutil.go(6 hunks)
💤 Files with no reviewable changes (1)
- pkg/wshutil/wshrouter.go
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: CodeQL
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (18)
pkg/util/ds/expmap.go (2)
31-40: Consider making the comparator function type-aware.
Usinganycasting might risk runtime panics if an unexpected type is ever passed in. A dedicated, typed comparator (or a safe runtime type-check) can help prevent accidental misuse.
60-79: Validate performance for large heaps.
For certain use cases, repeatedly callingexpireItems_nolock()could lead to performance bottlenecks when many items are near expiration. More sophisticated data structures or a background sweeper might improve performance.cmd/wsh/cmd/wshcmd-test.go (1)
18-20: Verify command integration with rootCmd.
Double-check thattestCmddoes not conflict with other hidden commands and thatrootCmdproperly handles top-level args whentestis invoked.pkg/wshrpc/wshserver/wshserverutil.go (1)
26-26: Confirm the “main-client” debugName usage.
Ensure that any downstream debugging or logging facility consistently uses"main-client"to identify this RPC client and continue verifying that calls exceeding 1s are properly handled or terminated, per the PR objective.pkg/wshrpc/wshclient/barerpcclient.go (1)
34-34: LGTM! Debug name addition aids in RPC timeout tracking.The addition of the debug name "bare-client" will help identify which RPC client is causing timeouts in the runServer loop, aligning with the PR's objective.
cmd/wsh/cmd/wshcmd-root.go (1)
89-89: LGTM! Consistent debug name additions for RPC tracking.The addition of descriptive debug names ("wshcmd-termclient" and "wshcmd") across all RPC client setup functions will help identify the source of any RPC calls that exceed the 1-second timeout limit.
Also applies to: 151-151, 169-169
cmd/wsh/cmd/wshcmd-connserver.go (1)
140-140: LGTM! Using route ID as debug name enables precise timeout tracking.Using
authRtn.RouteIdas the debug name is an excellent choice as it provides a unique identifier for each connection, making it easier to track down which specific connection is causing RPC timeouts.cmd/server/main-server.go (2)
199-199: LGTM! Debug name addition for local connection tracking.The addition of the "conn:local" debug name helps identify RPC calls from the local connection client.
199-201: Verify timeout handling implementation.While debug names have been added to track RPC calls, the actual timeout handling mechanism to fail calls that block for more than 1s is not visible in these changes. Please ensure that the timeout handling is implemented elsewhere.
Run the following script to locate the timeout implementation:
✅ Verification successful
Timeout handling is properly implemented
The timeout handling for RPC calls is implemented in pkg/wshutil/wshrpc.go:
- Blocking calls are tracked with 1s expiration
- Context timeout of 1s is enforced for RPC handling
- Response channel has timeout protection
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for timeout-related changes in the RPC implementation # Look for timeout-related code in RPC handling rg -A 5 'func.*RunServer|func.*HandleRpc|timeout.*1.*s|time\.Second' # Look for changes to RPC context or options ast-grep --pattern 'type RpcOpts struct { $$$ Timeout $_ $$$ }'Length of output: 29857
pkg/wshutil/wshrpc.go (3)
30-31: LGTM! Good use of expiring map for tracking blocking responses.Using
ds.ExpMapwith expiring entries is a good approach to prevent log spam when multiple responses are blocked.
186-189: LGTM! Enhanced RPC data structure.Adding
CommandandRoutefields torpcDataimproves error reporting by providing more context when RPC calls block.
756-783: Well-implemented blocking detection with timeout.The implementation properly handles blocking RPC responses:
- Attempts non-blocking send first
- Logs blocking occurrence (with rate limiting via expMap)
- Retries with 1s timeout
- Fails the RPC call if still blocked after timeout
This aligns well with the PR objective of failing blocked RPC calls.
pkg/wshutil/wshutil.go (4)
202-206: LGTM! Enhanced debugging for terminal RPC setup.Adding
debugStrparameter helps identify which RPC client is experiencing blocking issues.
224-228: LGTM! Consistent debug naming in packet RPC setup.The debug name parameter is consistently propagated to the underlying RPC client.
Line range hint
241-263: LGTM! Debug support for connection RPC setup.The connection RPC setup properly handles the debug name parameter.
Line range hint
275-284: LGTM! Debug support for domain socket RPC.The domain socket RPC setup correctly propagates the debug name.
pkg/waveapp/waveapp.go (1)
179-179: LGTM! Client properly identifies itself in RPC setup.Using "vdomclient" as the debug identifier helps track RPC blocking issues from the VDOM client.
go.mod (1)
12-12: Verify the usage of the gods library.The
github.com/emirpasic/godslibrary has been added, presumably to support the RPC timeout functionality. Let's verify its usage in the codebase.✅ Verification successful
The gods library is appropriately used for implementing an expiring map data structure
The library is used in
pkg/util/ds/expmap.goto implement a thread-safe map with automatic key expiration using a binary heap for efficient timeout tracking. This is a well-designed utility that could support various features including RPC timeout management.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check how the gods library is being used in the codebase # Expected: Find imports and usage of gods library in relation to RPC timeouts # Search for gods imports echo "=== Searching for gods imports ===" rg --type go "github.com/emirpasic/gods" # Search for specific data structures from gods that might be used for RPC tracking echo -e "\n=== Searching for potential gods data structures usage ===" rg --type go "gods\.(TreeMap|HashMap|Set|List)" # Look for new files that might use the gods library echo -e "\n=== Searching for recently modified files that might use gods ===" git diff --name-only | xargs rg --type go "gods\."Length of output: 570
Script:
#!/bin/bash # Check the implementation and usage of expmap echo "=== Content of expmap.go ===" cat pkg/util/ds/expmap.go echo -e "\n=== Files referencing expmap ===" rg --type go "expmap" echo -e "\n=== RPC timeout related code ===" rg --type go -i "rpc.*timeout|timeout.*rpc"Length of output: 10205
| func runTestCmd(cmd *cobra.Command, args []string) error { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement the actual test logic or remove placeholder code.
Returning nil suggests no operation performed. If the command is intended for testing system functionality, consider adding meaningful actions or removing the command to avoid confusion.
No description provided.