Welcome to mirror list, hosted at ThFree Co, Russian Federation.

gitlab.com/gitlab-org/gitaly.git - Unnamed repository; edit this file 'description' to name the repository.
summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorPatrick Steinhardt <psteinhardt@gitlab.com>2023-02-08 12:01:58 +0300
committerPatrick Steinhardt <psteinhardt@gitlab.com>2023-02-08 12:12:52 +0300
commite82b3a05940de3f2a8b03dd8a982ebd4295e3a01 (patch)
tree47d1865e0722bdf4740ba1804a771d574acf687e
parentfb0ec0b37a8acd90c5466f190496feeb68a5da74 (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.go12
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()