From ca53dc9b52eb9d804552c84c53184faaffc051c9 Mon Sep 17 00:00:00 2001 From: Eduard S Date: Wed, 24 Feb 2021 12:00:38 +0100 Subject: [PATCH] In http servers, first listen, then serve - In test, doing the `net.Listen` outside of the goroutine guarantees that we are listening, so we avoid a possible datarace consisting of doing an http request before listening. - In packages that run an http server: doing the listen first allows - Checking for errors when opening the address for listening before starting the goroutine, so that if there's an error on listen we can terminate grafecully - Logging that we are listening when we are really listening, and not before. --- api/api_test.go | 21 +++++++++++++++------ node/node.go | 11 ++++++++--- test/debugapi/debugapi.go | 13 +++++++++---- test/proofserver/proofserver.go | 11 ++++++++--- 4 files changed, 40 insertions(+), 16 deletions(-) diff --git a/api/api_test.go b/api/api_test.go index acdc282..4b1b175 100644 --- a/api/api_test.go +++ b/api/api_test.go @@ -8,6 +8,7 @@ import ( "io" "io/ioutil" "math/big" + "net" "net/http" "os" "strconv" @@ -38,8 +39,8 @@ type Pendinger interface { New() Pendinger } -const apiPort = ":4010" -const apiURL = "http://localhost" + apiPort + "/" +const apiAddr = ":4010" +const apiURL = "http://localhost" + apiAddr + "/" var SetBlockchain = ` Type: Blockchain @@ -241,9 +242,14 @@ func TestMain(m *testing.M) { panic(err) } // Start server - server := &http.Server{Addr: apiPort, Handler: apiGin} + listener, err := net.Listen("tcp", apiAddr) //nolint:gosec + if err != nil { + panic(err) + } + server := &http.Server{Handler: apiGin} go func() { - if err := server.ListenAndServe(); err != nil && tracerr.Unwrap(err) != http.ErrServerClosed { + if err := server.Serve(listener); err != nil && + tracerr.Unwrap(err) != http.ErrServerClosed { panic(err) } }() @@ -609,9 +615,12 @@ func TestTimeout(t *testing.T) { <-finishWait }) // Start server - serverTO := &http.Server{Addr: ":4444", Handler: apiGinTO} + serverTO := &http.Server{Handler: apiGinTO} + listener, err := net.Listen("tcp", ":4444") //nolint:gosec + require.NoError(t, err) go func() { - if err := serverTO.ListenAndServe(); err != nil && tracerr.Unwrap(err) != http.ErrServerClosed { + if err := serverTO.Serve(listener); err != nil && + tracerr.Unwrap(err) != http.ErrServerClosed { require.NoError(t, err) } }() diff --git a/node/node.go b/node/node.go index a7874e0..0243551 100644 --- a/node/node.go +++ b/node/node.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "net" "net/http" "sync" "time" @@ -444,16 +445,20 @@ func NewNodeAPI( // cancelation. func (a *NodeAPI) Run(ctx context.Context) error { server := &http.Server{ - Addr: a.addr, Handler: a.engine, // TODO: Figure out best parameters for production ReadTimeout: 30 * time.Second, //nolint:gomnd WriteTimeout: 30 * time.Second, //nolint:gomnd MaxHeaderBytes: 1 << 20, //nolint:gomnd } + listener, err := net.Listen("tcp", a.addr) + if err != nil { + return tracerr.Wrap(err) + } + log.Infof("NodeAPI is ready at %v", a.addr) go func() { - log.Infof("NodeAPI is ready at %v", a.addr) - if err := server.ListenAndServe(); err != nil && tracerr.Unwrap(err) != http.ErrServerClosed { + if err := server.Serve(listener); err != nil && + tracerr.Unwrap(err) != http.ErrServerClosed { log.Fatalf("Listen: %s\n", err) } }() diff --git a/test/debugapi/debugapi.go b/test/debugapi/debugapi.go index 9ff170f..a3b4db6 100644 --- a/test/debugapi/debugapi.go +++ b/test/debugapi/debugapi.go @@ -2,6 +2,7 @@ package debugapi import ( "context" + "net" "net/http" "time" @@ -118,16 +119,20 @@ func (a *DebugAPI) Run(ctx context.Context) error { debugAPI.GET("sync/stats", a.handleSyncStats) debugAPIServer := &http.Server{ - Addr: a.addr, Handler: api, - // Use some hardcoded numberes that are suitable for testing + // Use some hardcoded numbers that are suitable for testing ReadTimeout: 30 * time.Second, //nolint:gomnd WriteTimeout: 30 * time.Second, //nolint:gomnd MaxHeaderBytes: 1 << 20, //nolint:gomnd } + listener, err := net.Listen("tcp", a.addr) + if err != nil { + return tracerr.Wrap(err) + } + log.Infof("DebugAPI is ready at %v", a.addr) go func() { - log.Infof("DebugAPI is ready at %v", a.addr) - if err := debugAPIServer.ListenAndServe(); err != nil && tracerr.Unwrap(err) != http.ErrServerClosed { + if err := debugAPIServer.Serve(listener); err != nil && + tracerr.Unwrap(err) != http.ErrServerClosed { log.Fatalf("Listen: %s\n", err) } }() diff --git a/test/proofserver/proofserver.go b/test/proofserver/proofserver.go index 8f6f3b6..b995a53 100644 --- a/test/proofserver/proofserver.go +++ b/test/proofserver/proofserver.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "io/ioutil" + "net" "net/http" "sync" "time" @@ -202,16 +203,20 @@ func (s *Mock) Run(ctx context.Context) error { apiGroup.POST("/cancel", s.handleCancel) debugAPIServer := &http.Server{ - Addr: s.addr, Handler: api, // Use some hardcoded numberes that are suitable for testing ReadTimeout: 30 * time.Second, //nolint:gomnd WriteTimeout: 30 * time.Second, //nolint:gomnd MaxHeaderBytes: 1 << 20, //nolint:gomnd } + listener, err := net.Listen("tcp", s.addr) + if err != nil { + return tracerr.Wrap(err) + } + log.Infof("prover.MockServer is ready at %v", s.addr) go func() { - log.Infof("prover.MockServer is ready at %v", s.addr) - if err := debugAPIServer.ListenAndServe(); err != nil && tracerr.Unwrap(err) != http.ErrServerClosed { + if err := debugAPIServer.Serve(listener); err != nil && + tracerr.Unwrap(err) != http.ErrServerClosed { log.Fatalf("Listen: %s\n", err) } }()