diff options
author | Patrick Steinhardt <psteinhardt@gitlab.com> | 2023-02-08 12:01:58 +0300 |
---|---|---|
committer | Patrick Steinhardt <psteinhardt@gitlab.com> | 2023-02-08 12:12:52 +0300 |
commit | e82b3a05940de3f2a8b03dd8a982ebd4295e3a01 (patch) | |
tree | 47d1865e0722bdf4740ba1804a771d574acf687e | |
parent | fb0ec0b37a8acd90c5466f190496feeb68a5da74 (diff) |
grpc-proxy: Fix flake caused by race between serving and stopping
The error propagation tests for our gRPC proxy set up two gRPC servers,
the proxy server and the backend server. As we verify that errors are
correctly propagated at different leveles when proxying, some of the
subtests will never cause the backend server to be hit in case the proxy
server already retruns an error.
This can cause a race in our test setup between starting to serve and
tearing down the backend server when the backend server is never being
connected to. As nothing will synchronize the Goroutine that starts to
serve with the deferred function to stop it, they may be executed out of
order so that we first stop and then try to serve the server. This will
cause `grpc.Server.Serve()` to return an error:
=== FAIL: internal/praefect/grpc-proxy/proxy TestProxyErrorPropagation/director_error_is_propagated (0.01s)
handler_ext_test.go:358:
Error Trace: /home/pks/Development/gitlab/gdk/gitaly/internal/praefect/grpc-proxy/proxy/testhelper.go:123
/home/pks/Development/gitlab/gdk/gitaly/internal/praefect/grpc-proxy/proxy/handler_ext_test.go:358
/home/pks/Development/gitlab/gdk/gitaly/internal/praefect/grpc-proxy/proxy/asm_amd64.s:1594
Error: Received unexpected error:
grpc: the server has been stopped
Test: TestProxyErrorPropagation/director_error_is_propagated
--- FAIL: TestProxyErrorPropagation/director_error_is_propagated (0.01s)
Fix this by explicitly handling and ignoring `grpc.ErrServerStopped` in
this test. While we could move this error handling into `MustServe()`,
it does not feel right to do so as it indeed indicates a real error when
trying to serve. Furthermore, this error is really quite specific to
this test setup as you can typically expect to hit the gRPC server that
you're setting up.
-rw-r--r-- | internal/praefect/grpc-proxy/proxy/handler_ext_test.go | 12 |
1 files changed, 11 insertions, 1 deletions
diff --git a/internal/praefect/grpc-proxy/proxy/handler_ext_test.go b/internal/praefect/grpc-proxy/proxy/handler_ext_test.go index 36ab8bc53..d6daa391c 100644 --- a/internal/praefect/grpc-proxy/proxy/handler_ext_test.go +++ b/internal/praefect/grpc-proxy/proxy/handler_ext_test.go @@ -355,7 +355,17 @@ func TestProxyErrorPropagation(t *testing.T) { wg.Add(1) go func() { defer wg.Done() - testhelper.MustServe(t, backendServer, backendListener) + + err := backendServer.Serve(backendListener) + switch { + case errors.Is(err, grpc.ErrServerStopped): + // `grpc.Server.Serve()` may return `ErrServerStopped()` in + // case `Serve()` is called after `Stop()`. This can indeed + // happen in this test as the backend server will not get + // hit when the proxy server is already returning an error. + default: + require.NoError(t, err) + } }() defer backendServer.Stop() |