add changes based on feedback

Signed-off-by: Noah Kreiger <[email protected]>

v2/protocol/http/options.go

@@ -83,30 +83,32 @@ func WithShutdownTimeout(timeout time.Duration) Option {
 	}
 }
 
-// WithReadTimeout sets the read timeout for the http server. If not set, it will default to the
-// DefaultTimeout (600s). There is no maximum limit on the timeout for long-running processes.
+// WithReadTimeout overwrites the default read timeout (600s) of the http
+// server. The specified timeout must not be negative. A timeout of 0 disables
+// read timeouts in the http server.
 func WithReadTimeout(timeout time.Duration) Option {
 	return func(p *Protocol) error {
 		if p == nil {
 			return fmt.Errorf("http read timeout option can not set nil protocol")
 		}
 		if timeout < 0 {
-			return fmt.Errorf("http read timeout option can not be negative, for infinite timeouts we suggest setting an extremely high timeout")
+			return fmt.Errorf("http read timeout must not be negative")
 		}
 		p.readTimeout = &timeout
 		return nil
 	}
 }
 
-// WithWriteTimeout sets the write timeout for the http server. If not set, it will default to the
-// DefaultTimeout (600s). There is no maximum limit on the timeout for long-running processes.
+// WithWriteTimeout overwrites the default write timeout (600s) of the http
+// server. The specified timeout must not be negative. A timeout of 0 disables
+// write timeouts in the http server.
 func WithWriteTimeout(timeout time.Duration) Option {
 	return func(p *Protocol) error {
 		if p == nil {
 			return fmt.Errorf("http write timeout option can not set nil protocol")
 		}
 		if timeout < 0 {
-			return fmt.Errorf("http write timeout option can not be negative, for infinite timeouts we suggest setting an extremely high timeout")
+			return fmt.Errorf("http write timeout must not be negative")
 		}
 		p.writeTimeout = &timeout
 		return nil

v2/protocol/http/options_test.go

@@ -333,7 +333,7 @@ func TestWithReadTimeout(t *testing.T) {
 		"negative timeout": {
 			t:       &Protocol{},
 			timeout: -1,
-			wantErr: "http read timeout option can not be negative, for infinite timeouts we suggest setting an extremely high timeout",
+			wantErr: "http read timeout must not be negative",
 		},
 		"nil protocol": {
 			wantErr: "http read timeout option can not set nil protocol",
@@ -384,7 +384,7 @@ func TestWithWriteTimeout(t *testing.T) {
 		"negative timeout": {
 			t:       &Protocol{},
 			timeout: -1,
-			wantErr: "http write timeout option can not be negative, for infinite timeouts we suggest setting an extremely high timeout",
+			wantErr: "http write timeout must not be negative",
 		},
 		"nil protocol": {
 			wantErr: "http write timeout option can not set nil protocol",
@@ -489,9 +489,19 @@ func forceClose(tr *Protocol) {
 }
 
 func TestWithPort0(t *testing.T) {
+	noReadWriteTimeout := time.Duration(0)
+
 	testCases := map[string]func() (*Protocol, error){
-		"WithPort0": func() (*Protocol, error) { return New(WithPort(0)) },
-		"SetPort0":  func() (*Protocol, error) { return &Protocol{Port: 0}, nil },
+		"WithPort0": func() (*Protocol, error) {
+			return New(WithPort(0))
+		},
+		"SetPort0": func() (*Protocol, error) {
+			return &Protocol{
+				Port:         0,
+				readTimeout:  &noReadWriteTimeout,
+				writeTimeout: &noReadWriteTimeout,
+			}, nil
+		},
 	}
 	for name, f := range testCases {
 		t.Run(name, func(t *testing.T) {

v2/protocol/http/protocol.go

@@ -70,20 +70,16 @@ type Protocol struct {
 	// If 0, DefaultShutdownTimeout is used.
 	ShutdownTimeout time.Duration
 
-	// readTimeout defines the http.Server ReadTimeout
-	// It is the maximum duration for reading the entire
-	// request, including the body. A negative value means
-	// there will be no timeout.
-	// If 0, DefaultReadTimeout is used.
+	// readTimeout defines the http.Server ReadTimeout It is the maximum duration
+	// for reading the entire request, including the body. If not overwritten by an
+	// option, the default value (600s) is used
 	readTimeout *time.Duration
 
-	// writeTimeout defines the http.Server WriteTimeout
-	// It is the maximum duration before timing out
-	// writes of the response. It is reset whenever a new
-	// request's header is read. Like ReadTimeout, it does not
-	// let Handlers make decisions on a per-request basis.
-	// A negative value means there will be no timeout.
-	// If 0, DefaultWriteTimeout is used.
+	// writeTimeout defines the http.Server WriteTimeout It is the maximum duration
+	// before timing out writes of the response. It is reset whenever a new
+	// request's header is read. Like ReadTimeout, it does not let Handlers make
+	// decisions on a per-request basis. If not overwritten by an option, the
+	// default value (600s) is used
 	writeTimeout *time.Duration
 
 	// Port is the port configured to bind the receiver to. Defaults to 8080.
@@ -132,14 +128,15 @@ func New(opts ...Option) (*Protocol, error) {
 		p.ShutdownTimeout = DefaultShutdownTimeout
 	}
 
+	// use default timeout from abuse protection value
+	defaultTimeout := DefaultTimeout
+
 	if p.readTimeout == nil {
-		cast := DefaultTimeout
-		p.readTimeout = &cast
+		p.readTimeout = &defaultTimeout
 	}
 
 	if p.writeTimeout == nil {
-		cast := DefaultTimeout
-		p.writeTimeout = &cast
+		p.writeTimeout = &defaultTimeout
 	}
 
 	if p.isRetriableFunc == nil {

v2/protocol/http/protocol_lifecycle.go

@@ -38,9 +38,8 @@ func (p *Protocol) OpenInbound(ctx context.Context) error {
 	}
 
 	p.server = &http.Server{
-		Addr:    listener.Addr().String(),
-		Handler: attachMiddleware(p.Handler, p.middleware),
-		// Timeout values should always be set to either 0, the DefaultTimeout, or User Provided
+		Addr:         listener.Addr().String(),
+		Handler:      attachMiddleware(p.Handler, p.middleware),
 		ReadTimeout:  *p.readTimeout,
 		WriteTimeout: *p.writeTimeout,
 	}