client: use go-winio.DialPipe directly by thaJeztah · Pull Request #50125 · moby/moby

thaJeztah · 2025-06-02T12:28:34Z

relates to socket: deprecate DialPipe docker/go-connections#133

The go-connections package implementation is only a shallow wrapper around go-winio for named pipes; use the go-winio implementation directly.

- How to verify it

- Human readable description for the release notes

- A picture of a cute animal (not mandatory but encouraged)

thaJeztah · 2025-06-02T12:29:09Z

client/client_unix.go

+
+// dialPipeContext connects to a Windows named pipe. It is not supported on non-Windows.
+func dialPipeContext(ctx context.Context, string) (net.Conn, error) {
+	return nil, syscall.EAFNOSUPPORT


We could probably change this to ErrProtocolNotAvailable (or any other error?) 🤔

The go-connections package implementation is only a shallow wrapper around go-winio for named pipes; use the go-winio implementation directly. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah · 2025-06-05T10:04:40Z

client/client.go

-			return sockets.DialPipe(cli.addr, 32*time.Second)
+			ctx, cancel := context.WithTimeout(ctx, 32*time.Second)
+			defer cancel()
+			return dialPipeContext(ctx, cli.addr)


🫠 looks like testutil has its own separate implementation of all of this

moby/testutil/request/npipe_windows.go

Lines 10 to 12 in c29de52

func npipeDial(path string, timeout time.Duration) (net.Conn, error) {

return winio.DialPipe(path, &timeout)

}

moby/testutil/request/request.go

Lines 197 to 224 in c29de52

// SockConn opens a connection on the specified socket

func SockConn(timeout time.Duration, daemon string) (net.Conn, error) {

daemonURL, err := url.Parse(daemon)

if err != nil {

return nil, errors.Wrapf(err, "could not parse url %q", daemon)

}

var c net.Conn

switch daemonURL.Scheme {

case "npipe":

return npipeDial(daemonURL.Path, timeout)

case "unix":

return net.DialTimeout(daemonURL.Scheme, daemonURL.Path, timeout)

case "tcp":

if os.Getenv("DOCKER_TLS_VERIFY") != "" {

// Setup the socket TLS configuration.

tlsConfig, err := getTLSConfig()

if err != nil {

return nil, err

}

dialer := &net.Dialer{Timeout: timeout}

return tls.DialWithDialer(dialer, daemonURL.Scheme, daemonURL.Host, tlsConfig)

}

return net.DialTimeout(daemonURL.Scheme, daemonURL.Host, timeout)

default:

return c, errors.Errorf("unknown scheme %v (%s)", daemonURL.Scheme, daemon)

}

}

Copilot

Pull Request Overview

This PR replaces the shallow go-connections wrapper for Windows named pipes with direct calls to go-winio’s DialPipeContext, and provides a stub on non-Windows platforms.

Introduces dialPipeContext in client_windows.go to forward to winio.DialPipeContext with context support
Adds a stub dialPipeContext in client_unix.go that returns EAFNOSUPPORT
Updates the main dialer to use the new dialPipeContext (with timeout) instead of sockets.DialPipe

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
client/client_windows.go	Added `dialPipeContext` forwarding to `winio.DialPipeContext`
client/client_unix.go	Added unsupported stub for non-Windows `dialPipeContext`
client/client.go	Switched npipe case to use new `dialPipeContext` with timeout

Comments suppressed due to low confidence (2)

client/client_unix.go:15

Update this comment to accurately describe the stub behavior on non-Windows, e.g., that dialPipeContext always returns syscall.EAFNOSUPPORT on Unix.

// dialPipeContext connects to a Windows named pipe. It is not supported on non-Windows.

client/client_windows.go:15

[nitpick] Add unit tests for dialPipeContext to verify successful connections and context timeout handling on Windows.

func dialPipeContext(ctx context.Context, addr string) (net.Conn, error) {

thaJeztah · 2025-06-30T13:35:31Z

cc @vvoland @dmcgowan

thaJeztah added status/2-code-review kind/refactor PR's that refactor, or clean-up code labels Jun 2, 2025

thaJeztah commented Jun 2, 2025

View reviewed changes

client: use go-winio.DialPipe directly

accbfde

The go-connections package implementation is only a shallow wrapper around go-winio for named pipes; use the go-winio implementation directly. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah force-pushed the client_winio_dialpipe branch from 3bdd26c to accbfde Compare June 2, 2025 12:38

thaJeztah commented Jun 5, 2025

View reviewed changes

thaJeztah marked this pull request as ready for review June 6, 2025 11:36

thaJeztah requested a review from Copilot June 27, 2025 10:23

Copilot AI reviewed Jun 27, 2025

View reviewed changes

thaJeztah requested review from dmcgowan and vvoland June 27, 2025 17:15

dmcgowan approved these changes Jun 30, 2025

View reviewed changes

cpuguy83 approved these changes Jun 30, 2025

View reviewed changes

cpuguy83 merged commit 89eb408 into moby:master Jun 30, 2025
228 of 229 checks passed

thaJeztah deleted the client_winio_dialpipe branch June 30, 2025 20:17

thaJeztah mentioned this pull request Jul 1, 2025

socket: deprecate DialPipe docker/go-connections#133

Merged

dmcgowan mentioned this pull request Aug 5, 2025

Prepare release notes for client/v0.1.0-alpha.0 #50637

Merged

austinvazquez mentioned this pull request Aug 5, 2025

Bump api version in client and main module to v1.52.0-alpha.1 #50641

Merged

thaJeztah mentioned this pull request Aug 27, 2025

[28.x backport] client: use go-winio.DialPipe directly #50822

Merged

dmcgowan mentioned this pull request Sep 5, 2025

Prepare release notes for client/v0.1.0-beta.0 #50913

Merged

thaJeztah added this to the 29.0.0 milestone Oct 6, 2025

thaJeztah added the process/cherry-picked label Oct 6, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

client: use go-winio.DialPipe directly#50125

client: use go-winio.DialPipe directly#50125
cpuguy83 merged 1 commit intomoby:masterfrom
thaJeztah:client_winio_dialpipe

thaJeztah commented Jun 2, 2025

Uh oh!

thaJeztah Jun 2, 2025

Uh oh!

thaJeztah Jun 5, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

thaJeztah commented Jun 30, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	func npipeDial(path string, timeout time.Duration) (net.Conn, error) {
	return winio.DialPipe(path, &timeout)
	}

	// SockConn opens a connection on the specified socket
	func SockConn(timeout time.Duration, daemon string) (net.Conn, error) {
	daemonURL, err := url.Parse(daemon)
	if err != nil {
	return nil, errors.Wrapf(err, "could not parse url %q", daemon)
	}

	var c net.Conn
	switch daemonURL.Scheme {
	case "npipe":
	return npipeDial(daemonURL.Path, timeout)
	case "unix":
	return net.DialTimeout(daemonURL.Scheme, daemonURL.Path, timeout)
	case "tcp":
	if os.Getenv("DOCKER_TLS_VERIFY") != "" {
	// Setup the socket TLS configuration.
	tlsConfig, err := getTLSConfig()
	if err != nil {
	return nil, err
	}
	dialer := &net.Dialer{Timeout: timeout}
	return tls.DialWithDialer(dialer, daemonURL.Scheme, daemonURL.Host, tlsConfig)
	}
	return net.DialTimeout(daemonURL.Scheme, daemonURL.Host, timeout)
	default:
	return c, errors.Errorf("unknown scheme %v (%s)", daemonURL.Scheme, daemon)
	}
	}

Conversation

thaJeztah commented Jun 2, 2025

Uh oh!

thaJeztah Jun 2, 2025

Choose a reason for hiding this comment

Uh oh!

thaJeztah Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

thaJeztah commented Jun 30, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants