diff options
| author | Patrick Steinhardt <psteinhardt@gitlab.com> | 2022-08-11 13:34:05 +0300 |
|---|---|---|
| committer | Patrick Steinhardt <psteinhardt@gitlab.com> | 2022-09-23 10:02:55 +0300 |
| commit | 5a514bc991f1b1c727db0bffa0d7f2308d28566e (patch) | |
| tree | d1b36e353ee94eb77938ebe5fc3342f16c731c0f | |
| parent | de1d44320173415fcdba41a2b768b08bd6f3aa93 (diff) | |
http: Fix missing error checks for `Serve()` function
The `http.Server.Serve()` function will return an error when the HTTP
server has either failed unexpectedly or in case it wasn't properly shut
down. We don't check this error in many places though, which both causes
the errcheck linter to complain and hides issues we have with some of
our test setups.
Fix the missing error checks via a new `testhelper.MustServe()` helper
function that does the error checking for us. Drop the corresponding
linting exceptions.
| -rw-r--r-- | .golangci.yml | 1 | ||||
| -rw-r--r-- | internal/git/gittest/http_server.go | 3 | ||||
| -rw-r--r-- | internal/gitlab/test_server.go | 2 | ||||
| -rw-r--r-- | internal/testhelper/testhelper.go | 9 |
4 files changed, 11 insertions, 4 deletions
diff --git a/.golangci.yml b/.golangci.yml index 73200d48e..70546143e 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -70,7 +70,6 @@ linters-settings: - (net.Listener).Close # Serve - (*gitlab.com/gitlab-org/gitaly/v15/internal/praefect.ServerFactory).Serve - - (*net/http.Server).Serve forbidigo: forbid: # Tests and code which use timing-based setups have repeatedly resulted diff --git a/internal/git/gittest/http_server.go b/internal/git/gittest/http_server.go index d55f2987a..a0ac490f3 100644 --- a/internal/git/gittest/http_server.go +++ b/internal/git/gittest/http_server.go @@ -11,6 +11,7 @@ import ( "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly/v15/internal/git" + "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper" ) // HTTPServer starts an HTTP server with git-http-backend(1) as CGI handler. The repository is @@ -43,7 +44,7 @@ func HTTPServer(ctx context.Context, tb testing.TB, gitCmdFactory git.CommandFac }) } - go s.Serve(listener) + go testhelper.MustServe(tb, &s, listener) return listener.Addr().(*net.TCPAddr).Port, s.Close } diff --git a/internal/gitlab/test_server.go b/internal/gitlab/test_server.go index b23e4b251..3a6a78eb0 100644 --- a/internal/gitlab/test_server.go +++ b/internal/gitlab/test_server.go @@ -569,7 +569,7 @@ func startSocketHTTPServer(tb testing.TB, mux *http.ServeMux, tlsCfg *tls.Config TLSConfig: tlsCfg, } - go server.Serve(socketListener) + go testhelper.MustServe(tb, &server, socketListener) url := "http+unix://" + filename cleanup := func() { diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go index 3cabbb07e..dfe418b50 100644 --- a/internal/testhelper/testhelper.go +++ b/internal/testhelper/testhelper.go @@ -8,11 +8,13 @@ import ( "crypto/rand" "crypto/x509" "encoding/pem" + "errors" "fmt" "io" "math/big" mrand "math/rand" "net" + "net/http" "os" "os/exec" "path/filepath" @@ -113,7 +115,12 @@ type Server interface { // server in a Goroutine. func MustServe(tb testing.TB, server Server, listener net.Listener) { tb.Helper() - require.NoError(tb, server.Serve(listener)) + + // `http.Server.Serve()` is expected to return `http.ErrServerClosed`, so we special-case + // this error here. + if err := server.Serve(listener); err != nil && !errors.Is(err, http.ErrServerClosed) { + require.NoError(tb, err) + } } // CopyFile copies a file at the path src to a file at the path dst |