From a9617c1e4aa5126366bc344e6208ab561d62a742 Mon Sep 17 00:00:00 2001 From: Dustin Xie Date: Thu, 14 Jul 2022 10:03:32 -0700 Subject: [PATCH 1/3] [httputil] add ReadHeaderTimeout --- api/http.go | 15 ++++----- pkg/util/httputil/httputil.go | 51 ++++++++++++++++++++++++------ pkg/util/httputil/httputil_test.go | 12 +++---- 3 files changed, 53 insertions(+), 25 deletions(-) diff --git a/api/http.go b/api/http.go index 66579ba1bd..67e9d8b488 100644 --- a/api/http.go +++ b/api/http.go @@ -11,6 +11,7 @@ import ( apitypes "github.com/iotexproject/iotex-core/api/types" "github.com/iotexproject/iotex-core/pkg/log" + "github.com/iotexproject/iotex-core/pkg/util/httputil" ) type ( @@ -31,17 +32,13 @@ func NewHTTPServer(route string, port int, handler http.Handler) *HTTPServer { if port == 0 { return nil } - svr := &HTTPServer{ - svr: &http.Server{ - Addr: ":" + strconv.Itoa(port), - WriteTimeout: 30 * time.Second, - ReadHeaderTimeout: 10 * time.Second, - }, - } mux := http.NewServeMux() mux.Handle("/"+route, handler) - svr.svr.Handler = mux - return svr + + svr := httputil.Server(":"+strconv.Itoa(port), mux, httputil.HeaderTimeout(10*time.Second)) + return &HTTPServer{ + svr: &svr, + } } // Start starts the http server diff --git a/pkg/util/httputil/httputil.go b/pkg/util/httputil/httputil.go index d5dbea833b..58d464e2bb 100644 --- a/pkg/util/httputil/httputil.go +++ b/pkg/util/httputil/httputil.go @@ -9,20 +9,51 @@ import ( ) const ( - _connectionCount = 400 - _readTimeout = 35 * time.Second - _writeTimeout = 35 * time.Second - _idleTimeout = 120 * time.Second + _connectionCount = 400 + _readHeaderTimeout = 5 * time.Second + _readTimeout = 30 * time.Second + _writeTimeout = 30 * time.Second + _idleTimeout = 120 * time.Second ) +type ( + // ServerOption is a server option + ServerOption func(*serverConfig) + + serverConfig struct { + ReadHeaderTimeout time.Duration + ReadTimeout time.Duration + WriteTimeout time.Duration + IdleTimeout time.Duration + } +) + +// HeaderTimeout sets header timeout +func HeaderTimeout(h time.Duration) ServerOption { + return func(cfg *serverConfig) { + cfg.ReadHeaderTimeout = h + } +} + // Server creates a HTTP server with time out settings. -func Server(addr string, handler http.Handler) http.Server { +func Server(addr string, handler http.Handler, opts ...ServerOption) http.Server { + cfg := serverConfig{ + ReadHeaderTimeout: _readHeaderTimeout, + ReadTimeout: _readTimeout, + WriteTimeout: _writeTimeout, + IdleTimeout: _idleTimeout, + } + for _, opt := range opts { + opt(&cfg) + } + return http.Server{ - ReadTimeout: _readTimeout, - WriteTimeout: _writeTimeout, - IdleTimeout: _idleTimeout, - Addr: addr, - Handler: handler, + ReadHeaderTimeout: cfg.ReadHeaderTimeout, + ReadTimeout: cfg.ReadTimeout, + WriteTimeout: cfg.WriteTimeout, + IdleTimeout: cfg.IdleTimeout, + Addr: addr, + Handler: handler, } } diff --git a/pkg/util/httputil/httputil_test.go b/pkg/util/httputil/httputil_test.go index 29189ed983..edbdf92f4e 100644 --- a/pkg/util/httputil/httputil_test.go +++ b/pkg/util/httputil/httputil_test.go @@ -4,7 +4,6 @@ import ( "errors" "net/http" "testing" - "time" "github.com/stretchr/testify/require" ) @@ -15,11 +14,12 @@ func TestServer(t *testing.T) { addr := "myAddress" expectValue := http.Server{ - ReadTimeout: 35 * time.Second, - WriteTimeout: 35 * time.Second, - IdleTimeout: 120 * time.Second, - Addr: addr, - Handler: handler, + ReadHeaderTimeout: _readHeaderTimeout, + ReadTimeout: _readTimeout, + WriteTimeout: _writeTimeout, + IdleTimeout: _idleTimeout, + Addr: addr, + Handler: handler, } result := Server(addr, handler) require.Equal(t, expectValue, result) From 8410dee7be48741c5952bf5d0cd9c00b80f834c6 Mon Sep 17 00:00:00 2001 From: Dustin Xie Date: Thu, 14 Jul 2022 17:49:28 -0700 Subject: [PATCH 2/3] address comment --- api/http.go | 2 +- pkg/probe/probe.go | 2 +- pkg/util/httputil/httputil.go | 25 ++++++++++++------------- pkg/util/httputil/httputil_test.go | 11 ++++++----- server/itx/server.go | 2 +- 5 files changed, 21 insertions(+), 21 deletions(-) diff --git a/api/http.go b/api/http.go index 67e9d8b488..449cd71775 100644 --- a/api/http.go +++ b/api/http.go @@ -35,7 +35,7 @@ func NewHTTPServer(route string, port int, handler http.Handler) *HTTPServer { mux := http.NewServeMux() mux.Handle("/"+route, handler) - svr := httputil.Server(":"+strconv.Itoa(port), mux, httputil.HeaderTimeout(10*time.Second)) + svr := httputil.NewServer(":"+strconv.Itoa(port), mux, httputil.HeaderTimeout(10*time.Second)) return &HTTPServer{ svr: &svr, } diff --git a/pkg/probe/probe.go b/pkg/probe/probe.go index d88f9a514a..6484e174af 100644 --- a/pkg/probe/probe.go +++ b/pkg/probe/probe.go @@ -55,7 +55,7 @@ func New(port int, opts ...Option) *Server { mux.HandleFunc("/health", readiness) mux.Handle("/metrics", promhttp.Handler()) - s.server = httputil.Server(fmt.Sprintf(":%d", port), mux) + s.server = httputil.NewServer(fmt.Sprintf(":%d", port), mux) return s } diff --git a/pkg/util/httputil/httputil.go b/pkg/util/httputil/httputil.go index 58d464e2bb..15ccebcfd5 100644 --- a/pkg/util/httputil/httputil.go +++ b/pkg/util/httputil/httputil.go @@ -9,11 +9,7 @@ import ( ) const ( - _connectionCount = 400 - _readHeaderTimeout = 5 * time.Second - _readTimeout = 30 * time.Second - _writeTimeout = 30 * time.Second - _idleTimeout = 120 * time.Second + _connectionCount = 400 ) type ( @@ -28,6 +24,14 @@ type ( } ) +// DefaultServerConfig is the default server config +var DefaultServerConfig = serverConfig{ + ReadHeaderTimeout: 5 * time.Second, + ReadTimeout: 30 * time.Second, + WriteTimeout: 30 * time.Second, + IdleTimeout: 120 * time.Second, +} + // HeaderTimeout sets header timeout func HeaderTimeout(h time.Duration) ServerOption { return func(cfg *serverConfig) { @@ -35,14 +39,9 @@ func HeaderTimeout(h time.Duration) ServerOption { } } -// Server creates a HTTP server with time out settings. -func Server(addr string, handler http.Handler, opts ...ServerOption) http.Server { - cfg := serverConfig{ - ReadHeaderTimeout: _readHeaderTimeout, - ReadTimeout: _readTimeout, - WriteTimeout: _writeTimeout, - IdleTimeout: _idleTimeout, - } +// NewServer creates a HTTP server with time out settings. +func NewServer(addr string, handler http.Handler, opts ...ServerOption) http.Server { + cfg := DefaultServerConfig for _, opt := range opts { opt(&cfg) } diff --git a/pkg/util/httputil/httputil_test.go b/pkg/util/httputil/httputil_test.go index edbdf92f4e..bc40aa4ff9 100644 --- a/pkg/util/httputil/httputil_test.go +++ b/pkg/util/httputil/httputil_test.go @@ -4,6 +4,7 @@ import ( "errors" "net/http" "testing" + "time" "github.com/stretchr/testify/require" ) @@ -14,14 +15,14 @@ func TestServer(t *testing.T) { addr := "myAddress" expectValue := http.Server{ - ReadHeaderTimeout: _readHeaderTimeout, - ReadTimeout: _readTimeout, - WriteTimeout: _writeTimeout, - IdleTimeout: _idleTimeout, + ReadHeaderTimeout: 2 * time.Second, + ReadTimeout: DefaultServerConfig.ReadTimeout, + WriteTimeout: DefaultServerConfig.WriteTimeout, + IdleTimeout: DefaultServerConfig.IdleTimeout, Addr: addr, Handler: handler, } - result := Server(addr, handler) + result := NewServer(addr, handler, HeaderTimeout(2*time.Second)) require.Equal(t, expectValue, result) }) } diff --git a/server/itx/server.go b/server/itx/server.go index 1c550b6886..69633b6ad3 100644 --- a/server/itx/server.go +++ b/server/itx/server.go @@ -243,7 +243,7 @@ func StartServer(ctx context.Context, svr *Server, probeSvr *probe.Server, cfg c mux.Handle("/debug/pprof/trace", http.HandlerFunc(pprof.Trace)) port := fmt.Sprintf(":%d", cfg.System.HTTPAdminPort) - adminserv = httputil.Server(port, mux) + adminserv = httputil.NewServer(port, mux) defer func() { if err := adminserv.Shutdown(ctx); err != nil { log.L().Error("Error when serving metrics data.", zap.Error(err)) From 812113fde7660a79f4f17274ec10d04005e175f5 Mon Sep 17 00:00:00 2001 From: Dustin Xie Date: Fri, 15 Jul 2022 10:12:27 -0700 Subject: [PATCH 3/3] address comment 2 --- api/http.go | 3 +-- pkg/util/httputil/httputil.go | 4 ++-- pkg/util/httputil/httputil_test.go | 2 +- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/api/http.go b/api/http.go index 449cd71775..f652fa336c 100644 --- a/api/http.go +++ b/api/http.go @@ -27,7 +27,6 @@ type ( ) // NewHTTPServer creates a new http server -// TODO: move timeout into config func NewHTTPServer(route string, port int, handler http.Handler) *HTTPServer { if port == 0 { return nil @@ -35,7 +34,7 @@ func NewHTTPServer(route string, port int, handler http.Handler) *HTTPServer { mux := http.NewServeMux() mux.Handle("/"+route, handler) - svr := httputil.NewServer(":"+strconv.Itoa(port), mux, httputil.HeaderTimeout(10*time.Second)) + svr := httputil.NewServer(":"+strconv.Itoa(port), mux, httputil.ReadHeaderTimeout(10*time.Second)) return &HTTPServer{ svr: &svr, } diff --git a/pkg/util/httputil/httputil.go b/pkg/util/httputil/httputil.go index 15ccebcfd5..5d3aa63194 100644 --- a/pkg/util/httputil/httputil.go +++ b/pkg/util/httputil/httputil.go @@ -32,8 +32,8 @@ var DefaultServerConfig = serverConfig{ IdleTimeout: 120 * time.Second, } -// HeaderTimeout sets header timeout -func HeaderTimeout(h time.Duration) ServerOption { +// ReadHeaderTimeout sets header timeout +func ReadHeaderTimeout(h time.Duration) ServerOption { return func(cfg *serverConfig) { cfg.ReadHeaderTimeout = h } diff --git a/pkg/util/httputil/httputil_test.go b/pkg/util/httputil/httputil_test.go index bc40aa4ff9..2c98dc9b78 100644 --- a/pkg/util/httputil/httputil_test.go +++ b/pkg/util/httputil/httputil_test.go @@ -22,7 +22,7 @@ func TestServer(t *testing.T) { Addr: addr, Handler: handler, } - result := NewServer(addr, handler, HeaderTimeout(2*time.Second)) + result := NewServer(addr, handler, ReadHeaderTimeout(2*time.Second)) require.Equal(t, expectValue, result) }) }