Cache HTTP client transports #1047
Conversation
Signed-off-by: Anuraag Agrawal <[email protected]>
Signed-off-by: Anuraag Agrawal <[email protected]>
Signed-off-by: Anuraag Agrawal <[email protected]>
jhump
left a comment
jhump
left a comment
There was a problem hiding this comment.
This looks good. I left a few comments and things I think would be useful to improve before merging.
I see you only updated referenceclient. There is a second reference implementation that uses the grpc-go library (instead of connect-go), and it has the same issue (though, interestingly, it is missing a TODO in the same vein of the one that you addressed). I'm not suggesting you to remedy that one, too (unless you want to!). But maybe you could add a TODO over there, so we still have a record of similar work that needs to be done? Maybe something like so, around line 153 in grpcclient/client.go:
// TODO: We should cache the client conns here so that we're not
// creating a new one for each test case
| }} | ||
| case conformancev1.HTTPVersion_HTTP_VERSION_UNSPECIFIED: | ||
| return nil, errors.New("an HTTP version must be specified") | ||
| transport, err := transports.get(req) |
There was a problem hiding this comment.
I think it would be better to push down the above code (lines 164-178) into the transports.get method, and then have it return serverURL, in addition to the transport. That way, all details of transport creation are delegated, and you wouldn't have to create the TLS config more than once. Speaking of which, let's also move createTLSConfig and roundTripperFunc over to transports.go, since they are really implementation details of creating a transport.
| ForceAttemptHTTP2: true, | ||
| } | ||
| } else { | ||
| transport = &http2.Transport{ |
There was a problem hiding this comment.
While you're moving this code around, mind adding a TODO comment about net/http's recent support for H2C?
// TODO: Now that go 1.24 is required, we should use net/http's builtin support
// for H2Cand drop the dependency on golang.org/x/net/http2.
There was a problem hiding this comment.
Went ahead and switched over to the native support
| }} | ||
| case conformancev1.HTTPVersion_HTTP_VERSION_UNSPECIFIED: | ||
| return nil, errors.New("an HTTP version must be specified") | ||
| } |
There was a problem hiding this comment.
I see that you just mechanically copied this switch over. But maybe we can make a slight improvement on it while we're touching this code: it should have a default case that returns an error about the HTTP version being unknown. (Otherwise, such a situation will result in a panic, which would be harder to troubleshoot than a proper error message.)
Signed-off-by: Anuraag Agrawal <[email protected]>
| ForceAttemptHTTP2: true, | ||
| } | ||
| } else { | ||
| transport = &http2.Transport{ |
There was a problem hiding this comment.
Went ahead and switched over to the native support
| urlString := scheme + net.JoinHostPort(req.Host, strconv.Itoa(int(req.Port))) | ||
| serverURL, err := url.ParseRequestURI(urlString) | ||
| if err != nil { | ||
| return nil, nil, fmt.Errorf("invalid url: %s", urlString) |
There was a problem hiding this comment.
While moving this, I noticed the previous errors.New with a format string was probably not intended
| urlString := scheme + net.JoinHostPort(req.Host, strconv.Itoa(int(req.Port))) | ||
| serverURL, err := url.ParseRequestURI(urlString) | ||
| if err != nil { | ||
| return nil, nil, fmt.Errorf("invalid url: %s", urlString) |
2 participants
I have been investigating instability with certain python app servers in connect-python. In the end, it seems like some servers deal with many new connections in a short time better than others. And as there was a TODO here for it, I went ahead and implemented caching to keep connections down. With it, I found a test that was unstable become stable. For another one it's a bit hard to wire this change to CI which is where we've seen failures and I will check after/if this is merged - I expect it to become stable with no concurrency limit arg.
While we may need to examine production readiness in face of many connections more (I think currently conformance can issue hundreds in a few seconds which may be more than we'd hope for anyways), conformance tests in a virtualized CI runner is probably not where we want to do it.
/cc @stefanvanburen