feat: enforce proxy dial timeouts and improve timeout handling #98
Conversation
Summary of ChangesHello @appleboy, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the robustness of proxy connections by introducing explicit timeout enforcement and improved error handling. It ensures that SSH connections made through a proxy will properly respect configured timeouts, preventing indefinite waits. Furthermore, the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds exported ErrProxyDialTimeout and changes proxy dialing to use a cancellable context with a goroutine honoring ssh_conf.Timeout (or default). Connect returns ErrProxyDialTimeout on proxy dial timeout; Run detects it via errors.Is to set isTimeout. Adds tests covering timeout behavior and goroutine leakage. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant EasySSH
participant ProxyClient
participant Target
Caller->>EasySSH: Run/Connect(...)
alt proxy configured
EasySSH->>EasySSH: determine timeout (ssh_conf or default)
EasySSH->>EasySSH: create context with timeout
par async dial
EasySSH->>ProxyClient: Dial("tcp", target) (goroutine)
ProxyClient-->>EasySSH: conn or dial error
and timeout watcher
EasySSH->>EasySSH: wait for dial result or ctx.Done()
end
alt dial success
EasySSH->>Target: ssh.NewClientConn over conn
Target-->>EasySSH: SSH client/session
EasySSH-->>Caller: success
else ctx timeout
EasySSH-->>Caller: return ErrProxyDialTimeout (isTimeout=true)
end
else no proxy
EasySSH->>Target: direct SSH dial (with timeout)
Target-->>EasySSH: SSH client/session
EasySSH-->>Caller: success
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces timeout handling for proxy dialing, which is a great improvement. The implementation correctly uses a context to enforce the timeout on the proxyClient.Dial call. My review includes a few suggestions to improve the robustness of the error handling by using sentinel errors instead of string matching, and a note on a potential goroutine leak which is inherent to the timeout pattern used. I've also added some suggestions to make the new tests more specific and robust.
| go func() { | ||
| conn, err := proxyClient.Dial(string(ssh_conf.Protocol), net.JoinHostPort(ssh_conf.Server, ssh_conf.Port)) | ||
| connCh <- connResult{conn: conn, err: err} | ||
| }() |
There was a problem hiding this comment.
This goroutine may leak if the proxyClient.Dial call blocks indefinitely. When the select statement in the parent goroutine times out via ctx.Done(), this goroutine is not terminated and will continue running until proxyClient.Dial returns. If proxyClient.Dial never returns, the goroutine and its associated resources will be leaked.
While the underlying ssh library doesn't seem to provide a context-aware Dial method on the client, it's important to be aware of this potential resource leak.
easyssh.go
Outdated
| if strings.Contains(err.Error(), "proxy dial timeout") { | ||
| isTimeout = true | ||
| } |
There was a problem hiding this comment.
Checking for errors by matching substrings is brittle and can lead to bugs if the error message changes. A better practice is to use sentinel errors and check for them with errors.Is.
I suggest defining a specific error for proxy dial timeouts and using it consistently.
- Define a sentinel error at the package level (e.g., in the
varblock at the top of the file):
var ErrProxyDialTimeout = errors.New("proxy dial timeout")- In the
Connectfunction, wrap the error (line 282):
return nil, nil, fmt.Errorf("%w: %v", ErrProxyDialTimeout, ctx.Err())- Then, use
errors.Ishere to check for the error. If you prefer to stick with string matching for now, usingstrings.HasPrefixis slightly more specific and less prone to accidental matches thanstrings.Contains.
| if strings.Contains(err.Error(), "proxy dial timeout") { | |
| isTimeout = true | |
| } | |
| if strings.HasPrefix(err.Error(), "proxy dial timeout:") { | |
| isTimeout = true | |
| } |
| func TestProxyTimeoutHandling(t *testing.T) { | ||
| ssh := &MakeConfig{ | ||
| Server: "example.com", | ||
| User: "testuser", | ||
| Port: "22", | ||
| KeyPath: "./tests/.ssh/id_rsa", | ||
| Timeout: 1 * time.Second, // Short timeout for testing | ||
| Proxy: DefaultConfig{ | ||
| User: "testuser", | ||
| Server: "10.255.255.1", // Non-routable IP that should timeout | ||
| Port: "22", | ||
| KeyPath: "./tests/.ssh/id_rsa", | ||
| Timeout: 1 * time.Second, | ||
| }, | ||
| } | ||
|
|
||
| // Test Connect() method directly to test proxy connection timeout | ||
| start := time.Now() | ||
| session, client, err := ssh.Connect() | ||
| elapsed := time.Since(start) | ||
|
|
||
| // Should timeout within reasonable bounds | ||
| assert.True(t, elapsed < 3*time.Second, "Connection should timeout within 3 seconds, took %v", elapsed) | ||
| assert.True(t, elapsed >= 1*time.Second, "Connection should take at least 1 second (timeout value), took %v", elapsed) | ||
|
|
||
| // Should return nil session and client | ||
| assert.Nil(t, session) | ||
| assert.Nil(t, client) | ||
|
|
||
| // Should have error | ||
| assert.NotNil(t, err) | ||
| } |
There was a problem hiding this comment.
This is a good test for proxy connection timeouts. To make it more specific, you could also assert on the content of the error message to ensure it's a timeout error as expected. For example:
assert.ErrorContains(t, err, "i/o timeout")This would make the test more robust by verifying not just that an error occurred, but that the correct error occurred.
easyssh_test.go
Outdated
|
|
||
| // Should have timeout-related error | ||
| assert.NotNil(t, err) | ||
| assert.Contains(t, err.Error(), "proxy dial timeout") |
There was a problem hiding this comment.
This assertion will need to be updated if you adopt the suggestion of using sentinel errors for more robust error handling. With a sentinel error, you could use assert.ErrorIs for a more idiomatic and reliable check:
assert.ErrorIs(t, err, ErrProxyDialTimeout)![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 (1)
easyssh.go (1)
444-448: Consider using error wrapping instead of string matching.String matching for error detection is fragile. If the error message format changes, this detection will break.
Consider defining a custom error type and using error wrapping:
In the file, add:
type ProxyDialTimeoutError struct { Err error } func (e *ProxyDialTimeoutError) Error() string { return fmt.Sprintf("proxy dial timeout: %v", e.Err) } func (e *ProxyDialTimeoutError) Unwrap() error { return e.Err }Then in Connect() (line 282), wrap the error:
- return nil, nil, fmt.Errorf("proxy dial timeout: %v", ctx.Err()) + return nil, nil, &ProxyDialTimeoutError{Err: ctx.Err()}And in Run() (lines 444-448), use errors.As():
if err != nil { - // Check if the error is from a proxy dial timeout - if strings.Contains(err.Error(), "proxy dial timeout") { + var proxyTimeoutErr *ProxyDialTimeoutError + if errors.As(err, &proxyTimeoutErr) { isTimeout = true } return outStr, errStr, isTimeout, err }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
easyssh.go(2 hunks)easyssh_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
easyssh_test.go (1)
easyssh.go (2)
MakeConfig(47-76)DefaultConfig(79-103)
🔇 Additional comments (2)
easyssh_test.go (2)
516-549: Verify test reliability across different environments.The test uses a non-routable IP (10.255.255.1) to force a timeout. However, behavior may vary by OS and network configuration:
- Some systems may immediately fail with "network unreachable" instead of timing out
- Network behavior with non-routable IPs can be inconsistent
This could lead to flaky tests in CI/CD environments.
Consider running this test across different platforms (Linux, macOS, Windows) to verify consistent behavior. If it proves flaky, consider using a mock server or adjusting the test approach.
551-588: Document test prerequisites or make conditional.This test assumes SSH is running on localhost:22 (line 562). If SSH is not available, the test will fail when connecting to the proxy, which is unrelated to the proxy dial timeout being tested.
Consider one of the following:
- Document the prerequisite in test comments or README
- Skip the test conditionally if SSH is not available:
func TestProxyDialTimeout(t *testing.T) { // Check if SSH is available on localhost conn, err := net.DialTimeout("tcp", "127.0.0.1:22", 1*time.Second) if err != nil { t.Skip("SSH not available on localhost:22, skipping test") } conn.Close() // ... rest of test }Otherwise, the test provides good coverage of the proxy dial timeout scenario described in issue #93.
- Add support for proxy dial timeouts with a dedicated error type and detection. - Apply connection timeout logic when connecting through a proxy. - Update Run method to correctly set timeout flag if proxy dial timeout occurs. - Introduce tests to verify proxy timeouts and error handling on proxy connections. Signed-off-by: appleboy <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
easyssh.go (1)
261-288: Fix goroutine resource leak on proxy dial timeout.When the context times out at lines 286-287, the function returns immediately, but the goroutine at line 276 may still be executing
proxyClient.Dial(). If the dial eventually succeeds, the resulting connection will be sent toconnChbut never received or closed, causing a TCP connection leak.This issue was previously identified but not addressed. Apply this diff to close any connection established after timeout:
connCh := make(chan connResult, 1) go func() { conn, err := proxyClient.Dial(string(ssh_conf.Protocol), net.JoinHostPort(ssh_conf.Server, ssh_conf.Port)) connCh <- connResult{conn: conn, err: err} }() var conn net.Conn select { case result := <-connCh: conn = result.conn err = result.err case <-ctx.Done(): + // Drain the channel in background to prevent goroutine leak and close any connection established after timeout + go func() { + result := <-connCh + if result.conn != nil { + result.conn.Close() + } + }() return nil, nil, fmt.Errorf("%w: %v", ErrProxyDialTimeout, ctx.Err()) }easyssh_test.go (1)
589-628: Strengthen test assertions to ensure proxy dial timeout scenario is validated.The conditional check at lines 625-627 allows the test to pass even if
ErrProxyDialTimeoutis never triggered, which means the test might not be validating the intended scenario. Additionally, the test setup may not reliably produce a proxy dial timeout vs. a proxy connection timeout.Apply this diff to ensure the test validates the specific scenario:
- // Should have error assert.NotNil(t, err) - // If it's specifically a proxy dial timeout, isTimeout should be true - if errors.Is(err, ErrProxyDialTimeout) { - assert.True(t, isTimeout, "isTimeout should be true for proxy dial timeout") - } + // Should specifically be a proxy dial timeout error + assert.ErrorIs(t, err, ErrProxyDialTimeout, "Expected ErrProxyDialTimeout to be triggered") + // isTimeout should be true for proxy dial timeout + assert.True(t, isTimeout, "isTimeout should be true for proxy dial timeout")Additionally, consider using a test setup that more reliably triggers proxy dial timeout. The current setup with localhost proxy and non-routable target might timeout at the proxy connection phase instead. A more reliable approach would be to use a mock or a proxy that connects successfully but fails to dial the target within the timeout.
🧹 Nitpick comments (1)
easyssh_test.go (1)
517-550: Consider verifying the specific error type for more robust assertions.The test correctly validates timeout behavior and bounds, but doesn't verify the specific error type returned. Based on past review feedback, using
errors.Isorerrors.ErrorContainswould make the test more robust and specific.Apply this diff to add error type verification:
// Should have error assert.NotNil(t, err) + // Verify it's a proxy connection timeout error + assert.ErrorIs(t, err, ErrProxyDialTimeout)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
easyssh.go(3 hunks)easyssh_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
easyssh_test.go (1)
easyssh.go (3)
MakeConfig(52-81)DefaultConfig(84-108)ErrProxyDialTimeout(33-33)
🔇 Additional comments (3)
easyssh_test.go (1)
5-5: LGTM!The
errorsimport is correctly added to support theerrors.Ischeck inTestProxyDialTimeoutInRun.easyssh.go (2)
31-34: LGTM!The
ErrProxyDialTimeoutsentinel error is correctly defined as an exported variable, enabling callers to detect proxy dial timeouts usingerrors.Is. The naming and error message are clear and follow Go conventions.
449-453: LGTM!The error detection logic correctly identifies proxy dial timeout errors using
errors.Isand sets theisTimeoutflag accordingly. This addresses the requirement from issue #93 to properly surface timeout events to callers.
| // TestProxyDialTimeout tests the specific scenario described in issue #93 | ||
| // where proxy dial timeout should be respected and properly detected | ||
| func TestProxyDialTimeout(t *testing.T) { | ||
| ssh := &MakeConfig{ | ||
| Server: "10.255.255.1", // Non-routable IP that should timeout | ||
| User: "testuser", | ||
| Port: "22", | ||
| KeyPath: "./tests/.ssh/id_rsa", | ||
| Timeout: 2 * time.Second, // Short timeout for testing | ||
| Proxy: DefaultConfig{ | ||
| User: "testuser", | ||
| Server: "10.255.255.2", // Another non-routable IP for proxy | ||
| Port: "22", | ||
| KeyPath: "./tests/.ssh/id_rsa", | ||
| Timeout: 2 * time.Second, | ||
| }, | ||
| } | ||
|
|
||
| // Test Connect() method directly to avoid SSH server dependency | ||
| start := time.Now() | ||
| session, client, err := ssh.Connect() | ||
| elapsed := time.Since(start) | ||
|
|
||
| // Should timeout within reasonable bounds | ||
| assert.True(t, elapsed < 5*time.Second, "Connection should timeout within 5 seconds, took %v", elapsed) | ||
| assert.True(t, elapsed >= 2*time.Second, "Connection should take at least 2 seconds (timeout value), took %v", elapsed) | ||
|
|
||
| // Should return nil session and client | ||
| assert.Nil(t, session) | ||
| assert.Nil(t, client) | ||
|
|
||
| // Should have error | ||
| assert.NotNil(t, err) | ||
| // Note: This will timeout at the proxy connection level, not at proxy dial level | ||
| // so it won't be ErrProxyDialTimeout, but we can still verify the timeout behavior | ||
| } |
There was a problem hiding this comment.
Test does not verify what its name suggests.
The test name TestProxyDialTimeout implies it validates proxy dial timeout behavior, but the comment at lines 585-586 explicitly states "This will timeout at the proxy connection level, not at proxy dial level, so it won't be ErrProxyDialTimeout". This creates confusion about what scenario is actually being tested.
Consider one of the following:
- Rename the test to accurately reflect what it validates (e.g.,
TestProxyConnectionTimeout) - Or modify the test setup to actually trigger
ErrProxyDialTimeoutby ensuring the proxy connects successfully but fails to dial the target with the configured timeout
Apply this diff for option 1 (renaming):
-// TestProxyDialTimeout tests the specific scenario described in issue #93
-// where proxy dial timeout should be respected and properly detected
-func TestProxyDialTimeout(t *testing.T) {
+// TestProxyConnectionTimeout tests timeout behavior when connecting to proxy
+// This tests proxy connection timeout, not proxy dial timeout
+func TestProxyConnectionTimeout(t *testing.T) {Or for option 2, modify the test to use a working proxy but non-routable target:
ssh := &MakeConfig{
Server: "10.255.255.1", // Non-routable IP that should timeout
User: "testuser",
Port: "22",
KeyPath: "./tests/.ssh/id_rsa",
Timeout: 2 * time.Second, // Short timeout for testing
Proxy: DefaultConfig{
User: "testuser",
- Server: "10.255.255.2", // Another non-routable IP for proxy
+ Server: "localhost", // Use working proxy
Port: "22",
KeyPath: "./tests/.ssh/id_rsa",
Timeout: 2 * time.Second,
},
}Then update the comment and add an assertion for ErrProxyDialTimeout:
- // Should have error
assert.NotNil(t, err)
- // Note: This will timeout at the proxy connection level, not at proxy dial level
- // so it won't be ErrProxyDialTimeout, but we can still verify the timeout behavior
+ // Should specifically be a proxy dial timeout
+ assert.ErrorIs(t, err, ErrProxyDialTimeout)- Prevent goroutine leaks on proxy connection by handling context cancellation and closing connections if necessary - Add a test to verify that proxy dial timeouts do not cause goroutine leaks Signed-off-by: appleboy <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
easyssh_test.go (2)
518-551: Enhance error assertion specificity.The test validates timeout behavior but only checks that an error occurred. Consider asserting on the error type or message for more robust validation.
As per past review: Consider using
assert.ErrorContains(t, err, "i/o timeout")or similar to verify the correct error type occurred.
553-588: Test does not verify what its name suggests.The test name implies validation of proxy dial timeout behavior, but the comment at lines 586-587 explicitly states it will timeout at the proxy connection level, not the proxy dial level, so it won't produce
ErrProxyDialTimeout.As per past review: Either rename the test to reflect what it actually tests (e.g.,
TestProxyConnectionTimeout) or modify the setup to use a working proxy but non-routable target to actually triggerErrProxyDialTimeout.
🧹 Nitpick comments (1)
easyssh_test.go (1)
590-629: Add SSH availability check or document requirement
Tests assume an SSH server on localhost (27 occurrences), which may cause flakiness in environments without SSH. Add logic to detect and skip tests if unavailable or document this prerequisite.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
easyssh.go(3 hunks)easyssh_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
easyssh_test.go (1)
easyssh.go (3)
MakeConfig(52-81)DefaultConfig(84-108)ErrProxyDialTimeout(33-33)
🔇 Additional comments (5)
easyssh_test.go (2)
5-5: LGTM! Import additions are justified.The
errorsimport is needed forerrors.Ischecks inTestProxyDialTimeoutInRun, andruntimeis needed for goroutine leak detection inTestProxyGoroutineLeak.Also applies to: 9-9
631-667: Excellent addition for preventing resource leaks.This test effectively validates that the proxy dial timeout implementation doesn't leak goroutines, addressing a critical concern from past reviews. The approach is sound: capturing baseline goroutine counts, performing multiple timeout operations, allowing cleanup time, and verifying no significant growth occurred.
easyssh.go (3)
31-34: LGTM! Proper use of sentinel error pattern.The exported
ErrProxyDialTimeouterror enables callers to reliably detect proxy dial timeouts usingerrors.Is, which is superior to string-based error matching.
261-296: LGTM! Goroutine leak properly addressed.The implementation correctly prevents both goroutine and connection leaks:
- The goroutine at lines 276-287 uses a
selectstatement (lines 278-286) to either send the dial result or detect context cancellation and clean up- If
proxyClient.Dialcompletes after the timeout, the established connection is properly closed (line 284)- The buffered channel prevents the send from blocking if the main goroutine has already timed out
This design addresses the goroutine leak concern raised in past reviews.
457-461: LGTM! Correct timeout detection implementation.The use of
errors.Isproperly detectsErrProxyDialTimeouteven when wrapped, and correctly sets theisTimeoutflag to surface proxy dial timeout events to callers. This aligns with the PR objectives to ensure timeout detection is surfaced correctly.
1 participant
fix #93
Summary by CodeRabbit
New Features
Bug Fixes
Tests