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

net/http/httptest: optional faster test server #14200

Open
bradfitz opened this issue Feb 2, 2016 · 15 comments

Comments

Projects
None yet
9 participants
@bradfitz
Copy link
Member

commented Feb 2, 2016

Look into making httptest.Server have a way to use in-memory networking (i.e. net.Pipe) instead of localhost TCP. Might be faster/cheaper and not hit ephemeral port issues under load?

@bradfitz bradfitz self-assigned this Feb 2, 2016

@bradfitz bradfitz added this to the Go1.7 milestone Feb 2, 2016

@cmarcelo

This comment has been minimized.

Copy link
Contributor

commented Mar 3, 2016

@bradfitz have you started this?

I was looking how to do this and it seems we can hook the client side via a custom Dial function in Transport, and hook the server side via a custom net.Listener. Each new connection is backed by a net.Pipe.

A simplistic illustration of the hook points, completely ignoring Conn management and a bunch of stuff, can be seen in https://play.golang.org/p/l54WFN2dMW

@bradfitz bradfitz modified the milestones: Go1.8Maybe, Go1.7 May 10, 2016

@bradfitz bradfitz removed their assignment May 10, 2016

@bradfitz

This comment has been minimized.

Copy link
Member Author

commented May 10, 2016

@cmarcelo, no, I didn't start on this. I've written parts of this code a number of times in the net/http unit tests, but I've never packaged it all up nicely anywhere.

Feel free to investigate.

@DeedleFake

This comment has been minimized.

Copy link

commented Jul 15, 2016

I'm interested in doing this. It could be useful for a project I'm working on. Before I get started, I'd like to work out API details. I'm currently thinking of just adding a single, top-level function that returns both a Listener, for use in an httptest.Server, and an http.Transport.Dial implementation. If I'm understanding the code correctly, that should be feasible. Does that sound like a good way of doing it?

Edit: On second thought, that way of doing this wouldn't really be very HTTP-specific. Do you want this in the httptest package?

@bradfitz

This comment has been minimized.

Copy link
Member Author

commented Jul 15, 2016

This bug would involve zero API changes as I imagined it. If it does require API changes, it's probably out of scope for Go 1.x.

@DeedleFake

This comment has been minimized.

Copy link

commented Jul 15, 2016

By 'out of scope for Go 1.x', do you mean that it would break the Go 1 compatibility guarantee, or just that it can't make it into Go 1.7 because it's past the freeze? What I'm thinking of wouldn't break the guarantee, and I certainly don't expect changes now to make it into Go 1.7.

To clarify, I'm picturing a new function, probably in the httptest package, with a signature along the following lines:

func PipeNetwork(addr string, next func(net, addr string) (net.Conn, error)) (net.Listener, func(net, addr string) (net.Conn, error))

It would be used like this:

lis, dial := PipeNetwork(DefaultRemoteAddr, net.Dial)

s := httptest.NewUnstartedServer(someHandler)
s.Listener = lis
s.Start()

c := &http.Client{Transport: &http.Transport{Dial: dial}}

Basically, calls to dial where the addr parameter matched the addr parameter given to PipeNetwork would use net.Pipe and connect to the returned net.Listener, while other addresses would be passed through to the provided next function.

@bradfitz

This comment has been minimized.

Copy link
Member Author

commented Jul 15, 2016

I think that's just too much API to be worth it for Go 1.x, even once the tree opens for Go 1.8.

A new field on httptest.Server might be okay (if needed), but I'd really prefer to see zero API additions instead. I think this can still be fixed without API additions.

@DeedleFake

This comment has been minimized.

Copy link

commented Jul 15, 2016

How would you fix it without API additions? Even if the net.Listener that httptest.NewServer and the related functions create used pipes, you'd still need a special client in order to connect to it. If you want me to try to stick to just a new field, I could add one that provides the aforementioned dial function and gets filled in by the Server initialization functions.

@bradfitz

This comment has been minimized.

Copy link
Member Author

commented Jul 15, 2016

Fair point. I was thinking of some of net/http's tests where the Client/Transport are provided to the test. But it's too late for that. I suppose we could mess with http.DefaultTransport, but that's probably too magical and frail. So it'll have to be opt-in, which means new API. Sad.

Maybe it could just be a method on the unstarted server to return a Client, and that's it. And requesting that client opts you into the fast mode, once Start is called? Or more explicit might be to set a bool on the UnstartedServer, and then also add a Client() *http.Client method which works regardless of the bool.

@DeedleFake

This comment has been minimized.

Copy link

commented Jul 15, 2016

I like the method that sets up the server and returns a special client. One potential issue, though, which can be solved potentially with a parameter to the new method but I'm still trying to figure out a good way to do it: If the server's handler redirects to another URL, then the client needs a way to figure out that the request shouldn't use the pipe. I can think of a couple of solutions:

  • Use naive string matching on addresses in the dial function, and pass things through to net.Dial if it doesn't match.
  • Allow the user to pass a function that determines a match. This is the most configurable. Could default to one of the other options if the user passes nil.
  • Do something fancy with Server.URL. This one's the simplest from the user's point of view, but also the least configurable.
  • Don't worry about this at all, and just document that if the handler redirects then the new pipe-based system shouldn't be used. Easiest to implement, but makes it less of a drop-in replacement.
@nightlyone

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2016

How is this intended to work with timeouts? net.Pipe() doesn't support them AFAICS.

@bradfitz

This comment has been minimized.

Copy link
Member Author

commented Aug 19, 2016

It is not intended. It's a test server, for testing your handlers. If you need an end-to-end test with all the features, you can continue to use the net-based one.

@gopherbot

This comment has been minimized.

Copy link

commented Aug 20, 2016

CL https://golang.org/cl/27407 mentions this issue.

@quentinmit quentinmit added the NeedsFix label Oct 10, 2016

@rsc rsc modified the milestones: Go1.9, Go1.8Maybe Nov 2, 2016

@bradfitz bradfitz modified the milestones: Go1.10, Go1.9 May 24, 2017

@bradfitz

This comment has been minimized.

Copy link
Member Author

commented May 24, 2017

First this needs a design. The comments in https://golang.org/cl/27407 have some, but let's move it here.

Maybe the existing httptest.NewUnstartedServer + a new httptest.Server.StartPipeMode? And then it can be documented as requiring the new httptest.Server.Client() method, which was added recently.

@navytux

This comment has been minimized.

Copy link
Contributor

commented Oct 27, 2017

For the reference: some of us are using network of pipes for testing transparently via using unified Networker interface in services. The interface has implementations for at least TCP and Pipenet + TLS layer on top of each:

https://lab.nexedi.com/kirr/go123/commit/7f62584e
https://lab.nexedi.com/kirr/go123/commit/d3a7a196

@bradfitz bradfitz modified the milestones: Go1.10, Unplanned Nov 29, 2017

@mbana

This comment has been minimized.

Copy link

commented Mar 6, 2019

Just voicing interest in seeing this being available as well. Wishing you all the best in the implementation. A thing to note if you haven't already is some of us would want to use this whilst testing WebSockets as well.

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