Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposal: x/net/nettest: add MakeLocalPipe to construct local listener and connections #30984

Open
mdlayher opened this issue Mar 21, 2019 · 6 comments

Comments

@mdlayher
Copy link
Member

commented Mar 21, 2019

I'm currently implementing my own net.Conn type and am working on tests using nettest.TestConn: https://godoc.org/golang.org/x/net/nettest#TestConn.

The nettest.MakePipe function provides adequate functionality for my needs, but a fair amount of boilerplate is required to create a nettest.MakePipe that can:

  • set up a local net.Listener
  • dial and point a net.Conn at the listener
  • do appropriate cleanup and error handling

As it turns out, there's a pretty useful function to make a local pipe in one of the tests: https://go.googlesource.com/net/+/refs/heads/master/nettest/conntest_test.go

I propose we generalize and export this function for use with net.Listener and net.Conn implementations of any type. I'm happy to accept name suggestions, but my proposed signature and WIP documentation is as follows:

// MakeLocalPipe creates a local net.Listener and points another net.Conn at the
// listener, producing a MakePipe function suitable for use with TestConn.
// 
// The listen function should produce a net.Listener, but should not invoke Accept or
// any other methods on the listener. The dial function receives the address of the
// listener, and should produce a net.Conn pointed at the listener. If dial is nil,
// net.Dial will be invoked with the network and address information from addr.
func MakeLocalPipe(
    listen func() (net.Listener, error),
    dial func(addr net.Addr) (net.Conn, error),
) nettest.MakePipe

Usage is as follows:

Producing a local pipe with a TCP listener and connection, using net.Dial as a default dial function.

nettest.TestConn(t, nettest.MakeLocalPipe(
    func() (net.Listener, error) {
        return net.Listen("tcp", ":0")
    },
    // dial: net.Dial(addr.Network(), addr.String())
    nil,
))

Producing a local pipe with a custom listener and connection implemented outside of the stdlib (https://github.com/mdlayher/vsock)

nettest.TestConn(t, nettest.MakeLocalPipe(
    func() (net.Listener, error) {
        return vsock.Listen(0)
    },
    func(addr net.Addr) (net.Conn, error) {
        a := addr.(*vsock.Addr)
        return vsock.Dial(a.ContextID, a.Port)
    },
))

I don't mind putting this code in my own repository or similar if deemed unnecessary, but it seems like this would be an appropriate addition to nettest, since it enables easy setup of scaffolding for use with nettest.TestConn.

/cc @dsnet @mikioh

@gopherbot gopherbot added this to the Proposal milestone Mar 21, 2019

@gopherbot gopherbot added the Proposal label Mar 21, 2019

@mdlayher

This comment has been minimized.

Copy link
Member Author

commented Mar 21, 2019

Spoke with @acln0 and we came up with a possible alternative API that would have similar semantics, but would enable future extensibility as well. Something along the lines of:

type LocalPipe struct {
    // Same semantics, cannot be nil.
    Listen func() (net.Listener, error)

    // Same semantics: if nil, use net.Dial.
    Dial func(addr net.Addr) (net.Conn, error)

    // Room for future extensibility, like perhaps:
    //
    // If not nil, called to clean up any artifacts produced by the net.Listener, e.g.
    // UNIX socket files.
    // Cleanup func(l net.Listener) error
}

// MakePipe produces a MakePipe function using the configuration defined on LocalPipe.
func (lp *LocalPipe) MakePipe() nettest.MakePipe {}

However, we aren't sure that the extensibility would ever be needed.

I think it makes sense to keep the UNIX socket cleanup logic from the original test if net.Listener.Network() is "unix" or "unixgram". I think it also makes sense to require the caller to deal with cleanup of any other socket artifacts from unknown families on their own.

@acln0

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

I am in favor of this proposal. Having implemented this particular scaffolding logic in a number of different places already, it would be pleasant to have a canonical home for it.

With the alternative struct-based construction @mdlayher mentioned, perhaps a direct method is nicer:

func (lp LocalPipe) MakePipe() (c1, c2 net.Conn, close func(), err error)

Then, caller code makes use of the method value like so:

package vsock_test

func TestConn(t *testing.T) {
	vsocklp := netttest.LocalPipe{
		Listen: func() (net.Listener, error) {
			return vsock.Listen(0)
		},
		Dial: func(addr net.Addr) (net.Conn, error) {
			a := addr.(*vsock.Addr)
			return vsock.Dial(a.ContextID, a.Port)
		},
	}
	nettest.TestConn(t, vsocklp.MakePipe)
}
@rsc

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2019

Thoughts, @mikioh?

@rsc

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

Once @mikioh and @mdlayher agree here, this can be accepted.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

Ping, @mikioh and @mdlayher.
Note that the crypto/tls package tests had what amounts to a copy of the nettest/conntest_test.go TestTestConn body, and we had some reliability problems with it on some systems. I submitted a few CLs to deflake tests (https://golang.org/cl/184157), by not assuming that every dial would succeed. I don't know if that's necessary in general, but if it is then that may influence the API design.

@mdlayher

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

I'm happy to submit the code I have been using for this. I haven't heard anything from @mikioh in a while.

I can't say I've run into tests which sometimes fail to dial, but assuming both of my proposed hook functions continue to return errors, I wouldn't be opposed to adding a retry mechanism.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.