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.

Sign up for GitHub

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

Create memhttp package to debug flaky testcases #594

Merged
merged 23 commits into from
Nov 1, 2023
Merged

Create memhttp package to debug flaky testcases #594

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
4c43e4b
Debug flaky testcases with in memory network
emcfarlane Sep 18, 2023
1f1df5d
Fix lint
emcfarlane Sep 18, 2023
225aa0d
Add TODO for flaky test
emcfarlane Sep 18, 2023
27410bd
Use local networking for benchmarks
emcfarlane Sep 18, 2023
facfa53
Merge branch 'main' into ed/memtest
emcfarlane Sep 19, 2023
877b4df
Create memhttp and memhttp test packages
emcfarlane Oct 6, 2023
932533f
Fix race on RegisterShutdown
emcfarlane Oct 10, 2023
c057a62
Fix transport desc
emcfarlane Oct 10, 2023
f6a2bd7
Fix feedback
emcfarlane Oct 11, 2023
40594b7
Fix description
emcfarlane Oct 11, 2023
b9fccc6
Add Cleanup method test servers
emcfarlane Oct 13, 2023
dd98dd4
Revert moving options
emcfarlane Oct 13, 2023
716794b
Ensure response errors are reported consistently
emcfarlane Oct 22, 2023
ccd39d4
Document BlockUntilResponseReady behaviour
emcfarlane Oct 22, 2023
91afa1e
Ensure CloseWrite is called
emcfarlane Oct 23, 2023
ea743b3
Feedback remove changes to duplexHTTPCall
emcfarlane Oct 23, 2023
628915a
Add comment to clarify behaviour
emcfarlane Oct 23, 2023
23dc2da
Restrict log errors to New|Logger|Lshortfile
emcfarlane Oct 23, 2023
2ee0def
Document response close error handling
emcfarlane Oct 25, 2023
00d7f6a
Fix and document Close behaviour for pipe
emcfarlane Oct 25, 2023
758f889
Move responseBodyReady to group what it protects
emcfarlane Oct 25, 2023
fa4a554
Add CloseResponse checks in TestServer
emcfarlane Oct 25, 2023
7b062d1
Merge branch 'main' into ed/memtest
emcfarlane Nov 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .golangci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,3 +119,9 @@ issues:
- linters: [revive]
text: "^if-return: "
path: error_writer.go
# We want to set http.Server's logger
- linters: [forbidigo]
path: internal/memhttp
# We want to show examples with http.Get
- linters: [noctx]
path: internal/memhttp/memhttp_test.go
5 changes: 2 additions & 3 deletions client_example_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,8 @@ import (

func Example_client() {
logger := log.New(os.Stdout, "" /* prefix */, 0 /* flags */)
// Unfortunately, pkg.go.dev can't run examples that actually use the
// network. To keep this example runnable, we'll use an HTTP server and
// client that communicate over in-memory pipes. The client is still a plain
// To keep this example runnable, we'll use an HTTP server and client
// that communicate over in-memory pipes. The client is still a plain
// *http.Client!
var httpClient *http.Client = examplePingServer.Client()

Expand Down
19 changes: 6 additions & 13 deletions client_ext_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,14 @@ import (
"context"
"errors"
"net/http"
"net/http/httptest"
"strings"
"testing"

connect "connectrpc.com/connect"
"connectrpc.com/connect/internal/assert"
pingv1 "connectrpc.com/connect/internal/gen/connect/ping/v1"
"connectrpc.com/connect/internal/gen/connect/ping/v1/pingv1connect"
"connectrpc.com/connect/internal/memhttp/memhttptest"
)

func TestNewClient_InitFailure(t *testing.T) {
Expand Down Expand Up @@ -75,16 +75,13 @@ func TestClientPeer(t *testing.T) {
t.Parallel()
mux := http.NewServeMux()
mux.Handle(pingv1connect.NewPingServiceHandler(pingServer{}))
server := httptest.NewUnstartedServer(mux)
server.EnableHTTP2 = true
server.StartTLS()
t.Cleanup(server.Close)
server := memhttptest.NewServer(t, mux)

run := func(t *testing.T, unaryHTTPMethod string, opts ...connect.ClientOption) {
t.Helper()
client := pingv1connect.NewPingServiceClient(
server.Client(),
server.URL,
server.URL(),
connect.WithClientOptions(opts...),
connect.WithInterceptors(&assertPeerInterceptor{t}),
)
Expand All @@ -109,7 +106,7 @@ func TestClientPeer(t *testing.T) {
err = clientStream.Send(&pingv1.SumRequest{})
assert.Nil(t, err)
// server streaming
serverStream, err := client.CountUp(ctx, connect.NewRequest(&pingv1.CountUpRequest{}))
serverStream, err := client.CountUp(ctx, connect.NewRequest(&pingv1.CountUpRequest{Number: 1}))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change relevant? Why would the request data make this flaky or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It causes the server to error with invalid_argument: number must be positive: got 0

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. But was that not true before? Why wasn't this a problem before this PR? (I'm trying to understand why change it now.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test is checking in the interceptor that the peer address and peer protocol is set. It's testing each request style by sending an empty but valid request, however server stream is the only one that will cause the server handler to error. This can now report the server error instead of success. I don't think this is intended behaviour of the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not setting Number will cause the following only for grpcweb:

--- FAIL: TestClientPeer (0.00s)
    --- FAIL: TestClientPeer/grpcweb (0.07s)
        client_ext_test.go:112:
            assertion:  assert.Nil
            got:        unknown: http2: response body closed

The grpcweb error response is a header only response, need to investigate why this error is returned on a no body.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now flaky with:

            client_ext_test.go:113:
                assertion:      assert.Nil
                got:    unknown: io: read/write on closed pipe

Discard is reading from a closed response which is causing an io.ErrClosedPipe. This is probably due to the new memhttp. Need to investigate.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now flaky with:

By "now", do you mean after the last commit you pushed, which removed calls to response.Body.Close()?

Also, in that latest commit, who is the caller that is now responsible for calling Close? Is this done when StreamClientConn.CloseResponse() is called? Is there a chance some code path is failing to call that? (In particular, I'm suspicious why this would only happen for gRPC-Web and not others.)

Discard is reading from a closed response which is causing an io.ErrClosedPipe.

It seems like discard should be resilient against io.ErrClosedPipe and replace it with io.EOF when observed, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's due to the change in closing the pipe writer on Reader instead of Writer. net.Pipe will close both halves but the error is given on the opposite end. So closing the Reader made the Writer return nice io.EOF errors but the Reader to receive io.ErrClosedPipe. HTTP2 libraries set the request error to the response here

I'll need to write more testing around closing, and create an issue for testing with different latencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HTTP2 sets the error on the response then here

Copy link
Contributor Author

@emcfarlane emcfarlane Oct 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discard should never see io.ErrClosedPipe, however I've added context error checks to avoid issues with cancelation errors bubbling up to CloseResponse() errors. These are tested in TestServer tests.

Working and passing with -count=100 locally, can't see any more flaky test issues. Could you please re-review.

t.Cleanup(func() {
assert.Nil(t, serverStream.Close())
})
Expand Down Expand Up @@ -157,14 +154,10 @@ func TestGetNotModified(t *testing.T) {

mux := http.NewServeMux()
mux.Handle(pingv1connect.NewPingServiceHandler(&notModifiedPingServer{etag: etag}))
server := httptest.NewUnstartedServer(mux)
server.EnableHTTP2 = true
server.StartTLS()
t.Cleanup(server.Close)

server := memhttptest.NewServer(t, mux)
client := pingv1connect.NewPingServiceClient(
server.Client(),
server.URL,
server.URL(),
connect.WithHTTPGet(),
)
ctx := context.Background()
Expand Down
9 changes: 3 additions & 6 deletions client_get_fallback_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,12 @@ package connect
import (
"context"
"net/http"
"net/http/httptest"
"strings"
"testing"

"connectrpc.com/connect/internal/assert"
pingv1 "connectrpc.com/connect/internal/gen/connect/ping/v1"
"connectrpc.com/connect/internal/memhttp/memhttptest"
)

func TestClientUnaryGetFallback(t *testing.T) {
Expand All @@ -38,14 +38,11 @@ func TestClientUnaryGetFallback(t *testing.T) {
},
WithIdempotency(IdempotencyNoSideEffects),
))
server := httptest.NewUnstartedServer(mux)
server.EnableHTTP2 = true
server.StartTLS()
t.Cleanup(server.Close)
server := memhttptest.NewServer(t, mux)

client := NewClient[pingv1.PingRequest, pingv1.PingResponse](
server.Client(),
server.URL+"/connect.ping.v1.PingService/Ping",
server.URL()+"/connect.ping.v1.PingService/Ping",
WithHTTPGet(),
WithHTTPGetMaxURLSize(1, true),
WithSendGzip(),
Expand Down
8 changes: 3 additions & 5 deletions compression_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,10 @@ package connect
import (
"context"
"net/http"
"net/http/httptest"
"testing"

"connectrpc.com/connect/internal/assert"
"connectrpc.com/connect/internal/memhttp/memhttptest"
"google.golang.org/protobuf/types/known/emptypb"
)

Expand All @@ -42,12 +42,10 @@ func TestAcceptEncodingOrdering(t *testing.T) {
w.WriteHeader(http.StatusOK)
called = true
})
server := httptest.NewServer(verify)
t.Cleanup(server.Close)

server := memhttptest.NewServer(t, verify)
client := NewClient[emptypb.Empty, emptypb.Empty](
server.Client(),
server.URL,
server.URL(),
withFakeBrotli,
withGzip(),
)
Expand Down
Loading