fix: remove context from wsl connection struct #1663
Conversation
This fixes a bug where disconnecting the controller does not allow the controller to restart until the app reboots.
|
Warning Rate limit exceeded@oneirocosm has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 36 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe pull request introduces modifications to context management in the 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
🧹 Nitpick comments (2)
pkg/wsl/wsl.go (1)
190-196: Ensure proper context cancellation.This block properly creates and assigns a new
cancelFnwhile also ensuring any previouscancelFnis canceled first. However, consider wrapping the function body in a single lock acquisition to reduce the overhead of repeatedconn.WithLockcalls.190 ctx, cancelFn := context.WithCancel(context.Background()) conn.WithLock(func() { - if conn.cancelFn != nil { - conn.cancelFn() - } - conn.cancelFn = cancelFn + if conn.cancelFn != nil { conn.cancelFn() } + conn.cancelFn = cancelFn })pkg/shellexec/shellexec.go (1)
156-156: Graceful error handling.Verifying that
DetectShellhandles potential errors (e.g., no shell detected) is vital. The returned error is correctly propagated, but consider adding logs for debugging or user-friendly messages.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/shellexec/shellexec.go(2 hunks)pkg/wsl/wsl.go(5 hunks)
🔇 Additional comments (5)
pkg/wsl/wsl.go (3)
230-230: Validate concurrency around ConnController.
By assigning cmd := client.WslCommand(ctx, cmdStr) under this local context, you ensure that the command respects cancellations. Make sure references to cmd in other goroutines (e.g., the goroutine handling I/O) do not introduce race conditions.
482-482: Confirm initialization of new connection fields.
Initializing cancelFn to nil here is valid, but ensure other WslConn methods can handle a nil cancelFn gracefully.
Line range hint 209-219: Validate usage of context in downstream function calls.
GetWshPath and DetectShell rely on the passed context. Ensure they handle cancellation gracefully to prevent leaks and guarantee quick termination of any blocking I/O operations if cancellation is triggered.
Run the following script to confirm that GetWshPath and DetectShell have proper cancellation checks:
✅ Verification successful
Based on the code analysis, I can now provide a final response:
Context handling in GetWshPath and DetectShell is properly implemented
Both functions properly propagate the context through their command execution chains:
GetWshPathuses the context inWslCommandcalls to executewhich wshandwhere.exe wshDetectShelluses the context in bothGetWshPathand subsequentWslCommandcall- The underlying
WslCommandimplementation inwsl-win.gohas proper context validation:- Panics on nil context
- Propagates context to inner command execution
- Uses sync.WaitGroup for proper cleanup
The context is properly passed through the entire chain of command execution, ensuring that cancellation signals will be propagated to any running processes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Recursively search for context usage in GetWshPath and DetectShell
ast-grep --pattern $'function GetWshPath($_, $_) {\n $$$\n}'
ast-grep --pattern $'function DetectShell($_, $_) {\n $$$\n}'
Length of output: 122
Script:
#!/bin/bash
# Search for GetWshPath and DetectShell function definitions using ripgrep
echo "=== Searching for GetWshPath ==="
rg "func GetWshPath" -A 10
echo -e "\n=== Searching for DetectShell ==="
rg "func DetectShell" -A 10
Length of output: 2557
Script:
#!/bin/bash
# Search for WslCommand implementation to check context handling
echo "=== Searching for WslCommand method ==="
rg "func.*WslCommand" -A 10
# Search for any context cancellation patterns in the same files
echo -e "\n=== Checking context usage patterns ==="
rg "ctx\." pkg/wsl/wsl-util.go
Length of output: 1214
Script:
#!/bin/bash
# Search for Command implementation and context usage
echo "=== Searching for Command method ==="
rg "func.*Command.*Context" -A 10
# Search for exec.Command usage to see how commands are created
echo -e "\n=== Checking exec.Command usage ==="
rg "exec\.Command" pkg/wsl/
Length of output: 47433
pkg/shellexec/shellexec.go (2)
165-165: Ensure idempotent installation.
InstallClientRcFiles might partially install or update RC files. Ensure the function can gracefully handle repeated calls without corrupting the installation or leaving incomplete states if interrupted.
171-171: Context usage is consistent.
Using shellProcCtx here for GetHomeDir is consistent with the new local context approach. Confirm you don’t need the home directory beyond this method’s scope once canceled.
pkg/shellexec/shellexec.go
Outdated
| shellProcCtx, cancelFn := context.WithCancel(ctx) | ||
| defer cancelFn() |
There was a problem hiding this comment.
Consider controlling the lifetime beyond the function scope.
Creating a new context with its own cancel function and immediately deferring its cancellation will cancel the shell context as soon as this function returns, which limits the lifetime of the shell. If you intend for the shell to remain active after returning, remove or modify the defer cancelFn().
- shellProcCtx, cancelFn := context.WithCancel(ctx)
- defer cancelFn()
+ shellProcCtx, cancelFn := context.WithCancel(ctx)
// Possibly remove defer or invoke cancelFn() at a different timeCommittable suggestion skipped: line range outside the PR's diff.
|
might be able to add an actual timeout to some of these... not the connserver, but for the other utilities -- like shell detection or wsh path, etc. they could just have like a 5s timeout. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/wsl/wsl.go (2)
190-191: Consider increasing or externalizing the 2-second timeout.While a 2-second timeout may suffice in many scenarios, user feedback suggests that some WSL-related utilities (e.g., shell detection, path resolution) may benefit from a 5-second timeout. Extending or making this timeout configurable can help avoid premature timeouts under slow system conditions.
214-214: Ensure robust handling for DetectShell timeouts.Similar to
GetWshPath,DetectShellinvokes potentially time-consuming operations. If it exceeds the 2-second timeout, consider logging detailed context or offering a fallback shell path to avoid functionality blocks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/shellexec/shellexec.go(2 hunks)pkg/wsl/wsl.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/shellexec/shellexec.go
🔇 Additional comments (3)
pkg/wsl/wsl.go (3)
204-204: Validate potential context deadline errors from GetWshPath.Using
utilCtxwith a 2-second timeout might cause undesired failures ifGetWshPathoccasionally takes longer. Consider more extensive logging or a graceful fallback when a timeout occurs.
225-231: Confirm proper cleanup of old contexts.Overwriting the existing
cancelFncleans up previously running contexts, but ensure no running goroutines rely on the old context’s lifecycle. If tasks using the old context are still active, they may throw cancellation or incomplete operation errors. Otherwise, this is a good approach to prevent stale contexts.
484-484: Initialize cancelFn carefully to avoid nil calls.Currently,
cancelFndefaults toniland is later set byStartConnServer. This is fine as long as every usage site checks fornilfirst. Otherwise, consider assigning a no-op function to reduce branching.
This fixes a WSL bug where disconnecting the controller does not allow the controller to restart until the app reboots.