proxy: clientLoop; improve handling of closed pipes #126
Conversation
- use errors.Is to make sure we unwrap errors - also handle "syscall.WSAECONNRESET" (windows), and "syscall.ECONNRESET" (other platforms). Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
proxy_improve_errorhandling
branch
from
dc536f1 to
5705e79
Compare
There was a problem hiding this comment.
Pull Request Overview
Improves error handling in clientLoop by using errors.Is to detect both closed-pipe and connection-reset errors, and introduces platform-specific errConnReset constants.
- Switched from type assertion on
net.OpErrortoerrors.Ischecks forsyscall.EPIPEanderrConnReset. - Defined
errConnResetin OS-specific files (proxy_windows.go,proxy_unix.go).
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| proxy/tcp_proxy.go | Use errors.Is to detect EPIPE and errConnReset. |
| proxy/proxy_windows.go | Introduce errConnReset for Windows (WSAECONNRESET). |
| proxy/proxy_unix.go | Introduce errConnReset for Unix (ECONNRESET). |
Comments suppressed due to low confidence (2)
proxy/tcp_proxy.go:55
- [nitpick] The new error branch handling for
errConnResetandEPIPEshould be covered by unit or integration tests to ensure behavior is correct across platforms.
if errors.Is(err, syscall.EPIPE) || errors.Is(err, errConnReset) {
proxy/proxy_windows.go:1
- This file defines a Windows-specific constant but lacks a
//go:build windowsbuild tag at the top, leading to duplicate symbol errors on non-Windows platforms. Add//go:build windows(and// +build windowsfor older Go versions) above the package declaration.
package proxy
| @@ -51,7 +52,7 @@ func (proxy *TCPProxy) clientLoop(client *net.TCPConn, quit chan bool) { | |||
| if err != nil { | |||
| // If the socket we are writing to is shutdown with | |||
| // SHUT_WR, forward it to the other end of the pipe: | |||
| if err, ok := err.(*net.OpError); ok && err.Err == syscall.EPIPE { | |||
| if errors.Is(err, syscall.EPIPE) || errors.Is(err, errConnReset) { | |||
There was a problem hiding this comment.
docker-proxy seems to have its own version of this code.
Since https://github.com/moby/libnetwork/pull/1617/files, it just unconditionally closes the socket when io.Copy returns ... this should probably do the same?
There was a problem hiding this comment.
Good point; looking at that PR, I see that change was made following another PR that made other changes - are those relevant for this? Perhaps @AkihiroSuda recalls / could look just to be sure;
- A picture of a cute animal (not mandatory but encouraged)