Fix SOCKS5 DialContext signature and err reuse to unblock Windows build #8
Conversation
Co-authored-by: lwaay2025 <218015601+lwaay2025@users.noreply.github.com>
Co-authored-by: lwaay2025 <218015601+lwaay2025@users.noreply.github.com>
|
@copilot 支持http代理吗，单元测试都跑通了？编译没问题了？ |
There was a problem hiding this comment.
Pull request overview
This PR fixes two critical compilation errors that blocked the Windows build: a type mismatch in the SOCKS5 transport's DialContext function signature and a variable redeclaration error in the main startup code.
Key changes:
- Fixed SOCKS5
DialContextto use propercontext.Contexttype instead ofinterface{} - Added context-aware dialing with cancellation support for SOCKS5 proxies
- Changed
err := app.Run()toerr = app.Run()to reuse the existing error variable
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| services/httpclient.go | Fixed SOCKS5 DialContext signature, added base dialer with timeout, and implemented context-aware dial logic with goroutine-based cancellation handling |
| main.go | Fixed variable redeclaration by reusing existing err variable for app.Run() call |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if ctxErr := ctx.Err(); ctxErr != nil { | ||
| resultCh <- result{conn: nil, err: ctxErr} | ||
| return | ||
| } | ||
| conn, err := dialer.Dial(network, addr) | ||
| if ctx.Err() != nil && conn != nil { | ||
| _ = conn.Close() | ||
| err = ctx.Err() | ||
| conn = nil | ||
| } | ||
| resultCh <- result{conn: conn, err: err} | ||
| }() | ||
| select { | ||
| case res := <-resultCh: | ||
| return res.conn, res.err | ||
| case <-ctx.Done(): |
There was a problem hiding this comment.
There is a potential race condition in the context cancellation check. The goroutine checks ctx.Err() at line 220 and then calls dialer.Dial() at line 224, but the context could be canceled between these two checks. Additionally, even if the goroutine detects the context is canceled at line 225, it still sends the result to the channel, which could race with the ctx.Done() case in the select statement.
A cleaner approach would be to handle context cancellation in the select statement and let the goroutine complete naturally. The check at line 220 is also redundant since if the context is already done, the select statement will handle it immediately.
| if ctxErr := ctx.Err(); ctxErr != nil { | |
| resultCh <- result{conn: nil, err: ctxErr} | |
| return | |
| } | |
| conn, err := dialer.Dial(network, addr) | |
| if ctx.Err() != nil && conn != nil { | |
| _ = conn.Close() | |
| err = ctx.Err() | |
| conn = nil | |
| } | |
| resultCh <- result{conn: conn, err: err} | |
| }() | |
| select { | |
| case res := <-resultCh: | |
| return res.conn, res.err | |
| case <-ctx.Done(): | |
| conn, err := dialer.Dial(network, addr) | |
| resultCh <- result{conn: conn, err: err} | |
| }() | |
| select { | |
| case res := <-resultCh: | |
| // If the context was canceled while dialing, prefer the context error | |
| if ctxErr := ctx.Err(); ctxErr != nil { | |
| if res.conn != nil { | |
| _ = res.conn.Close() | |
| } | |
| return nil, ctxErr | |
| } | |
| return res.conn, res.err | |
| case <-ctx.Done(): | |
| // Context canceled before dialing completed; ensure we close any eventual connection. | |
| go func() { | |
| res := <-resultCh | |
| if res.conn != nil { | |
| _ = res.conn.Close() | |
| } | |
| }() |
| select { | ||
| case res := <-resultCh: | ||
| return res.conn, res.err | ||
| case <-ctx.Done(): | ||
| return nil, ctx.Err() | ||
| } |
There was a problem hiding this comment.
There is a potential resource leak when the context is canceled. If the context is canceled after the select statement returns at line 235-236 but before the goroutine completes and sends the result to the channel, the connection opened by dialer.Dial() at line 224 may not be closed. The goroutine would send a result to the buffered channel, but since the main function has already returned, no one would receive it to close the connection.
Consider adding cleanup logic to ensure connections are always closed when the context is canceled, even if the cancellation happens after the function returns.
| } | ||
| conn, err := dialer.Dial(network, addr) | ||
| if ctx.Err() != nil && conn != nil { | ||
| _ = conn.Close() |
There was a problem hiding this comment.
The error returned by conn.Close() is being silently ignored with _. While it's common to ignore close errors in cleanup paths, in this case the connection closure is part of the cancellation handling logic. If the close operation fails, it could lead to resource leaks.
Consider logging the error or at minimum using a comment to document why the error is intentionally ignored.
| DialContext: func(ctx context.Context, network, addr string) (net.Conn, error) { | ||
| if ctxDialer, ok := dialer.(proxy.ContextDialer); ok { | ||
| return ctxDialer.DialContext(ctx, network, addr) | ||
| } | ||
| type result struct { | ||
| conn net.Conn | ||
| err error | ||
| } | ||
| resultCh := make(chan result, 1) | ||
| go func() { | ||
| if ctxErr := ctx.Err(); ctxErr != nil { | ||
| resultCh <- result{conn: nil, err: ctxErr} | ||
| return | ||
| } | ||
| conn, err := dialer.Dial(network, addr) | ||
| if ctx.Err() != nil && conn != nil { | ||
| _ = conn.Close() | ||
| err = ctx.Err() | ||
| conn = nil | ||
| } | ||
| resultCh <- result{conn: conn, err: err} | ||
| }() | ||
| select { | ||
| case res := <-resultCh: | ||
| return res.conn, res.err | ||
| case <-ctx.Done(): | ||
| return nil, ctx.Err() | ||
| } |
There was a problem hiding this comment.
The new context-aware SOCKS5 dialing logic lacks test coverage. Given that the repository has comprehensive tests for other services (geminiservice_test.go, providerservice_test.go, etc.), this critical proxy functionality should also have test coverage to verify context cancellation behavior, timeout handling, and connection cleanup work correctly.
Consider adding tests for scenarios such as:
- Successful dial with context
- Context canceled before dial completes
- Context canceled after dial completes
- Type assertion to ContextDialer succeeds/fails
Windows release build failed due to a SOCKS5 transport DialContext type mismatch and a redeclared
errwhen running the app.DialContextwithcontext.Context, added cancellation-aware dialing with timeout-backed base dialer, and ensured late connections are closed when the context is canceled.errvariable forapp.Run()to remove the redeclaration compile error.Example (context-aware SOCKS5 dialing):
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.