From 0c0a78dd3b30d65f8900748e9130913fcd5134a4 Mon Sep 17 00:00:00 2001 From: Norio Nomura Date: Mon, 20 Oct 2025 23:53:43 +0900 Subject: [PATCH 1/2] pkg/portfwdserver: Close `stream` by returning from the handler method The documentation says: https://pkg.go.dev/google.golang.org/grpc#BidiStreamingServer > To terminate the stream, return from the handler method and return an error from the status package, or use nil to indicate an OK status code. - Changed to calling `proxy.HandleConn()`/`bicopy.Bicopy()` as goroutines, then wait `GRPCServerRW.closeCh` in `TunnelServer.Start()` for returning from handler before `HandleConn()`/`bicopy.Bicopy()` finishes. - Added `CloseRead()` and `CloseWrite()` to `GRPCServerRW`. As a result, `GRPCServerRW` may be expected to pass the test added by #3708 with inetaf/tcpproxy's `tcpproxy.DialProxy()` or `bicopy.Bicopy()`. Signed-off-by: Norio Nomura --- pkg/portfwd/client.go | 3 +++ pkg/portfwdserver/server.go | 38 ++++++++++++++++++++++++++++++++----- 2 files changed, 36 insertions(+), 5 deletions(-) diff --git a/pkg/portfwd/client.go b/pkg/portfwd/client.go index 5090f20b268..b3435296180 100644 --- a/pkg/portfwd/client.go +++ b/pkg/portfwd/client.go @@ -21,6 +21,7 @@ import ( func HandleTCPConnection(_ context.Context, dialContext func(ctx context.Context, network string, addr string) (net.Conn, error), conn net.Conn, guestAddr string) { proxy := tcpproxy.DialProxy{Addr: guestAddr, DialContext: dialContext} proxy.HandleConn(conn) + logrus.Debugf("tcp proxy for guestAddr: %s closed", guestAddr) } func HandleUDPConnection(ctx context.Context, dialContext func(ctx context.Context, network string, addr string) (net.Conn, error), conn net.PacketConn, guestAddr string) { @@ -39,6 +40,7 @@ func HandleUDPConnection(ctx context.Context, dialContext func(ctx context.Conte } }() proxy.Run() + logrus.Debugf("udp proxy for guestAddr: %s closed", guestAddr) } func DialContextToGRPCTunnel(client *guestagentclient.GuestAgentClient) func(ctx context.Context, network, addr string) (net.Conn, error) { @@ -97,6 +99,7 @@ func (g *GrpcClientRW) Read(p []byte) (n int, err error) { } func (g *GrpcClientRW) Close() error { + logrus.Debugf("closing GrpcClientRW for id: %s", g.id) return g.stream.CloseSend() } diff --git a/pkg/portfwdserver/server.go b/pkg/portfwdserver/server.go index 295af993706..4a8a51cd752 100644 --- a/pkg/portfwdserver/server.go +++ b/pkg/portfwdserver/server.go @@ -13,6 +13,7 @@ import ( "time" "github.com/containers/gvisor-tap-vsock/pkg/tcpproxy" + "github.com/sirupsen/logrus" "github.com/lima-vm/lima/v2/pkg/bicopy" "github.com/lima-vm/lima/v2/pkg/guestagent/api" @@ -41,7 +42,11 @@ func (s *TunnelServer) Start(stream api.GuestService_TunnelServer) error { if err != nil { return err } - rw := &GRPCServerRW{stream: stream, id: in.Id} + rw := &GRPCServerRW{stream: stream, id: in.Id, closeCh: make(chan any, 1)} + go func() { + <-ctx.Done() + rw.Close() + }() // FIXME: consolidate bicopy and tcpproxy into one // @@ -51,20 +56,26 @@ func (s *TunnelServer) Start(stream api.GuestService_TunnelServer) error { // However, the tcpproxy package can't pass the CI for WSL2 (experimental): // https://github.com/lima-vm/lima/pull/3686#issuecomment-3034842616 if wsl2, _ := seemsWSL2(); wsl2 { - bicopy.Bicopy(rw, conn, nil) + go bicopy.Bicopy(rw, conn, nil) } else { proxy := tcpproxy.DialProxy{DialContext: func(_ context.Context, _, _ string) (net.Conn, error) { return conn, nil }} - proxy.HandleConn(rw) + go proxy.HandleConn(rw) } + // The stream will be closed when this function returns. + // Wait here until rw.Close(), rw.CloseRead(), or rw.CloseWrite() is called. + // We can't close rw.closeCh since the calling order of Close* methods is not guaranteed. + <-rw.closeCh + logrus.Debugf("closed GRPCServerRW for id: %s", in.Id) return nil } type GRPCServerRW struct { - id string - stream api.GuestService_TunnelServer + id string + stream api.GuestService_TunnelServer + closeCh chan any } var _ net.Conn = (*GRPCServerRW)(nil) @@ -84,6 +95,23 @@ func (g *GRPCServerRW) Read(p []byte) (n int, err error) { } func (g *GRPCServerRW) Close() error { + logrus.Debugf("closing GRPCServerRW for id: %s", g.id) + g.closeCh <- struct{}{} + return nil +} + +// By adding CloseRead and CloseWrite methods, GRPCServerRW can work with +// other than containers/gvisor-tap-vsock/pkg/tcpproxy, e.g., inetaf/tcpproxy, bicopy.Bicopy. + +func (g *GRPCServerRW) CloseRead() error { + logrus.Debugf("closing read GRPCServerRW for id: %s", g.id) + g.closeCh <- struct{}{} + return nil +} + +func (g *GRPCServerRW) CloseWrite() error { + logrus.Debugf("closing write GRPCServerRW for id: %s", g.id) + g.closeCh <- struct{}{} return nil } From 34fa30529b7f3eabf24b704ee8f03dde52e5b9f1 Mon Sep 17 00:00:00 2001 From: Norio Nomura Date: Tue, 21 Oct 2025 20:57:09 +0900 Subject: [PATCH 2/2] pkg/portfwdserver: Remove workaround for WSL2 Signed-off-by: Norio Nomura --- .github/workflows/test.yml | 4 ++-- hack/test-templates.sh | 2 +- pkg/portfwdserver/server.go | 33 +++++---------------------------- 3 files changed, 8 insertions(+), 31 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index a54074b31dd..b95f5972638 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -177,7 +177,7 @@ jobs: - name: Integration tests (WSL2, Windows host) run: | $env:PATH = "$pwd\_output\bin;" + 'C:\msys64\usr\bin;' + $env:PATH - pacman -Sy --noconfirm openbsd-netcat diffutils socat + pacman -Sy --noconfirm openbsd-netcat diffutils socat w3m $env:MSYS2_ENV_CONV_EXCL = 'HOME_HOST;HOME_GUEST;_LIMA_WINDOWS_EXTRA_PATH' $env:HOME_HOST = $(cygpath.exe "$env:USERPROFILE") $env:HOME_GUEST = "/mnt$env:HOME_HOST" @@ -206,7 +206,7 @@ jobs: - name: Integration tests (QEMU, Windows host) run: | $env:PATH = "$pwd\_output\bin;" + 'C:\msys64\usr\bin;' + 'C:\Program Files\QEMU;' + $env:PATH - pacman -Sy --noconfirm openbsd-netcat diffutils socat + pacman -Sy --noconfirm openbsd-netcat diffutils socat w3m $env:MSYS2_ENV_CONV_EXCL = 'HOME_HOST;HOME_GUEST;_LIMA_WINDOWS_EXTRA_PATH' $env:HOME_HOST = $(cygpath.exe "$env:USERPROFILE") $env:HOME_GUEST = "$env:HOME_HOST" diff --git a/hack/test-templates.sh b/hack/test-templates.sh index d786d6dfd94..e71897f23da 100755 --- a/hack/test-templates.sh +++ b/hack/test-templates.sh @@ -489,7 +489,7 @@ if [[ -n ${CHECKS["port-forwards"]} ]]; then limactl shell "$NAME" $sudo $CONTAINER_ENGINE rm -f nginx fi fi - if [[ ${NAME} != "alpine"* && ${NAME} != "wsl2"* ]] && command -v w3m >/dev/null; then + if [[ ${NAME} != "alpine"* ]] && command -v w3m >/dev/null; then INFO "Testing https://github.com/lima-vm/lima/issues/3685 ([gRPC portfwd] client connection is not closed immediately when server closed the connection)" # Skip the test on Alpine, as systemd-run is missing # Skip the test on WSL2, as port forwarding is half broken https://github.com/lima-vm/lima/pull/3686#issuecomment-3034842616 diff --git a/pkg/portfwdserver/server.go b/pkg/portfwdserver/server.go index 4a8a51cd752..77a2bbe8e4e 100644 --- a/pkg/portfwdserver/server.go +++ b/pkg/portfwdserver/server.go @@ -8,14 +8,11 @@ import ( "errors" "io" "net" - "os" - "strings" "time" "github.com/containers/gvisor-tap-vsock/pkg/tcpproxy" "github.com/sirupsen/logrus" - "github.com/lima-vm/lima/v2/pkg/bicopy" "github.com/lima-vm/lima/v2/pkg/guestagent/api" ) @@ -48,21 +45,11 @@ func (s *TunnelServer) Start(stream api.GuestService_TunnelServer) error { rw.Close() }() - // FIXME: consolidate bicopy and tcpproxy into one - // - // The bicopy package does not seem to work with `w3m -dump`: - // https://github.com/lima-vm/lima/issues/3685 - // - // However, the tcpproxy package can't pass the CI for WSL2 (experimental): - // https://github.com/lima-vm/lima/pull/3686#issuecomment-3034842616 - if wsl2, _ := seemsWSL2(); wsl2 { - go bicopy.Bicopy(rw, conn, nil) - } else { - proxy := tcpproxy.DialProxy{DialContext: func(_ context.Context, _, _ string) (net.Conn, error) { - return conn, nil - }} - go proxy.HandleConn(rw) - } + proxy := tcpproxy.DialProxy{DialContext: func(_ context.Context, _, _ string) (net.Conn, error) { + return conn, nil + }} + go proxy.HandleConn(rw) + // The stream will be closed when this function returns. // Wait here until rw.Close(), rw.CloseRead(), or rw.CloseWrite() is called. // We can't close rw.closeCh since the calling order of Close* methods is not guaranteed. @@ -134,13 +121,3 @@ func (g *GRPCServerRW) SetReadDeadline(_ time.Time) error { func (g *GRPCServerRW) SetWriteDeadline(_ time.Time) error { return nil } - -// seemsWSL2 returns whether lima.env contains LIMA_CIDATA_VMTYPE=wsl2 . -// This is a temporary workaround and has to be removed. -func seemsWSL2() (bool, error) { - b, err := os.ReadFile("/mnt/lima-cidata/lima.env") - if err != nil { - return false, err - } - return strings.Contains(string(b), "LIMA_CIDATA_VMTYPE=wsl2"), nil -}