x/sys/windows/namedpipe: new package

[bradfitz]

Update, Dec 8 2021: Proposed API is in #49650 (comment) -rsc

---

https://go-review.googlesource.com/c/sys/+/299009 is a CL to add a new "namedpipe" package to x/sys/windows/namedpipe.

This is an accompanying proposal for its API addition.

From that CL's commit message:

It is based on WireGuard's namedpipe library, which in turn is based on
parts of Microsoft's go-winio library.

The proposed API can more be seen here, which already exists outside of x/:

https://pkg.go.dev/golang.zx2c4.com/wireguard/ipc/namedpipe

It's basically a Unix socket dia/listen API but over Windows named pipes.

/cc @zx2c4 @jstarks

[zx2c4]

Until the issues regarding x/sys/windows have been discussed and concluded, I have no plans to maintain this code and therefore I can't support this proposal.

[ianlancetaylor]

@zx2c4 Is there a place where those issues are being discussed? Thanks.

[zx2c4]

Russ' comments on https://go-review.googlesource.com/c/sys/+/299009

[ianlancetaylor]

I'm sorry, I wrote a reply on that CL yesterday but forget to hit send.

[ianlancetaylor]

Why both DialContext and DialTimeout? Can the user implement DialTimeout by just using context.WithTimeout and calling DialContext? I would expect most current networking code to be using a context anyhow these days.

The fact that the new net package has both timeout and context variants is historical, not because we think it's a great idea.

[rsc]

For concreteness, here is the API from the external package:

func DialContext(ctx context.Context, path string) (net.Conn, error)
func DialTimeout(path string, timeout time.Duration) (net.Conn, error)
func Listen(path string) (net.Listener, error)

type DialConfig struct {
	ExpectedOwner *windows.SID // If non-nil, the pipe is verified to be owned by this SID.
}
func (config *DialConfig) DialContext(ctx context.Context, path string) (net.Conn, error)
func (config *DialConfig) DialTimeout(path string, timeout time.Duration) (net.Conn, error)

type ListenConfig struct {
	SecurityDescriptor *windows.SECURITY_DESCRIPTOR

	// MessageMode determines whether the pipe is in byte or message mode. In either
	// case the pipe is read in byte mode by default. The only practical difference in
	// this implementation is that CloseWrite is only supported for message mode pipes;
	// CloseWrite is implemented as a zero-byte write, but zero-byte writes are only
	// transferred to the reader (and returned as io.EOF in this implementation)
	// when the pipe is in message mode.
	MessageMode bool

	// InputBufferSize specifies the initial size of the input buffer, in bytes, which the OS will grow as needed.
	InputBufferSize int32

	// OutputBufferSize specifies the initial size of the output buffer, in bytes, which the OS will grow as needed.
	OutputBufferSize int32
}

func (c *ListenConfig) Listen(path string) (net.Listener, error)

Initial feedback:

• net has Dialer and ListenConfig. That's somewhat inconsistent, but we should probably do the same here, to avoid introducing a different kind of inconsistency. That is, DialConfig should probably be Dialer.

• As Ian pointed out, we should drop DialTimeout and Dialer.DialTimeout.

• It might be worth renaming DialContext to just Dial.

• Should Listen accept a context as well?

• I don't understand what "in either case" means in the doc comment for MessageMode.

• Why are the buffer sizes int32 instead of int?

[zx2c4]

Why both DialContext and DialTimeout? Can the user implement DialTimeout by just using context.WithTimeout and calling DialContext? I would expect most current networking code to be using a context anyhow these days.

We can ditch the DialTimeout one if you like, and just do DialContext and then,

It might be worth renaming DialContext to just Dial.

and then do that.

Does it then make sense to add Context as an optional member of DialConfig, and have the nil value default to a context.Background() with a two second timeout? Or would this be very non-standard and weird, and the whole idiom is to pass around contexts as arguments from one function to another? I suspect the latter, but in case the former actually would be nice, thought I should at least throw it out there.

net has Dialer and ListenConfig. That's somewhat inconsistent, but we should probably do the same here, to avoid introducing a different kind of inconsistency. That is, DialConfig should probably be Dialer.

Will do.

Should Listen accept a context as well?

I guess that'd be technically possible. The idea being - what if you want to just listen for 2 seconds and then have the listener automatically close? Sounds like a somewhat plausible use case. Do other listeners take a context?

I don't understand what "in either case" means in the doc comment for MessageMode.

Will clarify.

Why are the buffer sizes int32 instead of int?

Because they're passed to NtCreateNamedPipeFile, which takes a uint32. So actually they should be uint32s. I'll fix that.

[zx2c4]

In getting rid of DialTimeout and going Context-only, one thing I'm noticing is having to change a lot of checks for os.ErrDeadlineExceeded into context.DeadlineExceeded. I don't really suppose that's a problem, but wanted to note it somewhere public in case it comes up down the road.

[zx2c4]

The CL has been updated now with the above changes. Running go doc -all now produces:

package namedpipe // import "golang.org/x/sys/windows/namedpipe"

Package namedpipe implements a net.Conn and net.Listener around Windows
named pipes.

FUNCTIONS

func Dial(ctx context.Context, path string) (net.Conn, error)
    Dial calls Dialer.Dial using an empty configuration.

func Listen(ctx context.Context, path string) (net.Listener, error)
    Listen calls ListenConfig.Listen using an empty configuration.


TYPES

type Dialer struct {
        ExpectedOwner *windows.SID // If non-nil, the pipe is verified to be owned by this SID.
}
    Dialer exposes various options for use in Dial.

func (config *Dialer) Dial(ctx context.Context, path string) (net.Conn, error)
    Dial attempts to connect to the specified named pipe by path. It is
    advisable to pass a Context that has a timeout, which, for most use cases,
    is generally around 2 seconds.

type ListenConfig struct {
        // SecurityDescriptor contains a Windows security descriptor. If nil, the default from RtlDefaultNpAcl is used.
        SecurityDescriptor *windows.SECURITY_DESCRIPTOR

        // MessageMode determines whether the pipe is in byte or message mode. In both
        // cases the pipe is read in byte mode. The only practical difference in
        // this implementation is that CloseWrite is only supported for message mode pipes;
        // CloseWrite is implemented as a zero-byte write, but zero-byte writes are only
        // transferred to the reader (and returned as io.EOF in this implementation)
        // when the pipe is in message mode.
        MessageMode bool

        // InputBufferSize specifies the initial size of the input buffer, in bytes, which the OS will grow as needed.
        InputBufferSize uint32

        // OutputBufferSize specifies the initial size of the output buffer, in bytes, which the OS will grow as needed.
        OutputBufferSize uint32
}
    ListenConfig contains configuration for the pipe listener.

func (c *ListenConfig) Listen(ctx context.Context, path string) (net.Listener, error)
    Listen creates a listener on a Windows named pipe path,such as
    \\.\pipe\mypipe. The pipe must not already exist.

[gopherbot]

Change https://golang.org/cl/299009 mentions this issue: windows/namedpipe: add simple named pipe library

[jstarks]

InputBufferSize specifies the initial size of the input buffer, in bytes, which the OS will grow as needed.

I think I recall that this is not accurate--I believe npfs uses this as a quota for the maximum number of bytes that can be queued. Writes by the client will be pended in the kernel until there is sufficient quota or a matching reader is present who can consume the data directly.

(Same for OutputBufferSize, of course.)

[zx2c4]

Thanks. Will update.

[rsc]

Thanks for working on this API.
Does anyone object to the API in #49650 (comment) ?

[rsc]

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

[rsc]

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

[rsc]

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

[zx2c4]

Bringing the Gerrit discussion over here because it's easier to discuss here. @ianlancetaylor @bufflig @jstarks

The code in CL 299009 is partially © Microsoft. Based on the earlier discussion, it seems like we can do one of:

a) Submit it now, and hope at some point we get the go-ahead from Microsoft to remove their copyright header in a future CL.
b) Submit it now, and decide this doesn't actually matter.
c) Submit it later, after having received the go-ahead from Microsoft to remove their copyright header.

I'd like to actually use this code from x/sys, so I'm leaning in favor of (a) or (b), but maybe there are opinions to the contrary. How shall we proceed?

[ianlancetaylor]

I would hope to hear from @jstarks or somebody else at Microsoft. Given the holidays maybe if we haven't heard by early January we can consider option (a).

[zx2c4]

@ianlancetaylor It's January and no word from @jstarks, so can we go with option (a) now please?

[ianlancetaylor]

Let's give it another week. Thanks.

[jstarks]

No update yet.

[jstarks]

Things are looking good; give me a few days to work out the details.

[zx2c4]

@ianlancetaylor

Let's give it another week. Thanks.

9 days later, so option (a), and then we'll hear back from @jstarks sometime later and change things then?

[ianlancetaylor]

I know you want to get this done but is there a special rush here? Since it seems like we are making progress I'd rather take the time to get it right. Thanks.

[zx2c4]

Over a month has passed. Not wanting this to crust over, can we finally merge it? Microsoft's legal stuff will move at the pace that it does and we can change things up at that later date.

[zx2c4]

@ianlancetaylor Paging again after another month has passed. Can we merge this please?

[ianlancetaylor]

@jstarks Any update?

@zx2c4 I understand the frustration, I do. But speaking purely personally I would vote against submitting code that is copyright Microsoft into the main Go repos. That will cause undesirable long-term pain. So I don't want to follow a process of "check it in and then maybe change it later if we can." I'm sorry that this is so frustrating. I've personally spent multiple years waiting for copyright issues to clear in other cases, so I know what it's like.

[zx2c4]

It sounds like your opinion in #49650 (comment) has changed then, when you said we could maybe do option (a) in January? To be clear, it's fine if your opinion has changed, but it'd be useful to me to make that clear so I can manage my expectations.

[ianlancetaylor]

Yes, I suppose my opinion has changed. Sorry about that.

[qmuntal]

I'll ping @jstarks internally, he probably doesn't review GitHub notifications frequently.

[zx2c4]

7 months later, can we merge this finally?

[zx2c4]

@ianlancetaylor ping?

[ianlancetaylor]

@qmuntal @jstarks Any progress here? Thanks.

[qmuntal]

Little progress here. I've started a new thread with legal to internally revamp this. Can't commit to a deadline because I'm not in the sign off chain, just trying to make things happen.