Navigation Menu

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 · 16 comments
Open

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

bradfitz opened this issue Feb 2, 2016 · 16 comments
Labels
FeatureRequest help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@bradfitz
Copy link
Contributor

bradfitz 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
Copy link
Contributor

cmarcelo 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
Copy link
Contributor Author

@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
Copy link

DeedleFake 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
Copy link
Contributor Author

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
Copy link

DeedleFake 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
Copy link
Contributor Author

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
Copy link

DeedleFake 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
Copy link
Contributor Author

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
Copy link

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
Copy link
Contributor

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

@bradfitz
Copy link
Contributor Author

bradfitz 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
Copy link

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

@quentinmit quentinmit added the NeedsFix The path to resolution is known, but the work has not been done. 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
Copy link
Contributor Author

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
Copy link
Contributor

navytux 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
Copy link

mbana 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.

@mstrYoda

This comment was marked as duplicate.

emcfarlane added a commit to connectrpc/connect-go that referenced this issue Sep 18, 2023
Changes httptest.Server to use an in memory network. This makes testing
more robust to ephemeral port issues under load. Two flaky test cases
were now easily reproducible and fixed. Both occured from races between
starting the network request in a go routine and checking for a http2
stream and erroring if not. This can error on Send or Receive depending
on how fast the request can complete.

See: golang/go#14200
emcfarlane added a commit to connectrpc/connect-go that referenced this issue Sep 18, 2023
Changes httptest.Server to use an in memory network. This makes testing
more robust to ephemeral port issues under load. Two flaky test cases
were now easily reproducible and fixed. Both occured from races between
starting the network request in a go routine and checking for a http2
stream and erroring if not. This can error on Send or Receive depending
on how fast the request can complete.

See: golang/go#14200
emcfarlane added a commit to emcfarlane/go that referenced this issue Oct 6, 2023
Adds a new server initialization function to create a net.Pipe based
listener for a httptest.Server.

Fixes golang#14200.
akshayjshah pushed a commit to connectrpc/connect-go that referenced this issue Nov 1, 2023
Creates two internal packages: `memhttp` and `memhttptest` based on
https://github.com/akshayjshah/memhttp . We use this for testing
connect's services.

Changes`httptest.Server` to use an in memory network. This makes testing
more robust to ephemeral port issues under load. Two flaky test cases
were now easily reproducible and fixed. Both occurred from races between
starting the network request in a go routine and checking for a http2
stream and erroring if not. This can error on Send or Receive depending
on how fast the request can complete.

On `duplexHTTPCall` I removed `SetError` to avoid overwriting the
original response error. Now `BlockUntilResponseReady` returns an error
from the response initialization.

See: golang/go#14200

Fixes:
- `TestServer/http1/*/*/cumsum_cancel_before_send`: send can error on
write message.
- `TestBidiRequiresHTTP2`: send can `io.EOF` error on write, or succeed.
- `TestBidiOverHTTP1`: same as above.
- `TestClientPeer/grpcweb`: stream request sends an invalidate payload
invoking server error, races with interceptor.

Two places where errors raced:
-
https://github.com/connectrpc/connect-go/blob/525cf76ebd3c1c54f2cfd2cb4413c44b57539be9/duplex_http_call.go#L290
-
https://github.com/connectrpc/connect-go/blob/525cf76ebd3c1c54f2cfd2cb4413c44b57539be9/handler.go#L184
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FeatureRequest help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

10 participants