Allow Context to Configure Default Timeout

[nkreiger]

Adds options to the Protocol to set the Read and Write on the HTTP Client Handler for Cloud Events.

Resolves Issue: #1041

[embano1]

Thx for the contribution! I would recommend following our Options pattern here e.g.,

func WithHTTPTimeouts(readTimeout, writeTimeout time.Duration) Option {...}

I feel this need is similar to setting the ShutdownTimeout implemented here. However, please do not make the read/write fields public ;)

[nkreiger]

@embano1 will do, FYI may not have scope to do this for a while, I needed this for an emergency fix, however, as soon as I do I will circle back to update this PR. Feel free to push to it if you'd like

[embano1]

No worries, we'll hold until you have time

[nkreiger]

@embano1 I've updated the PR to use the protocol, added test coverage. Let me know if you need anything else. Thanks for waiting

Cheers

[nkreiger]

@embano1 I've made some changes, let me know what you think.

I left the Read and Write timeout checks on == 0. To make this backwards compatible with how the current defaults are set, as far as I can tell, that needs to be done. I've updated the comment to be more clear, that if you want to set an indefinite/no timeout, then it should be done by passing a negative value, and that 0 will default to the default timeouts.

[embano1]

Regarding your question on defaults/0/infinite: I see two options here (IMHO we don't need the -1 case):

1. You follow what ShutdownTimeout is doing where 0 == default - if someone wants to set infinite they can use a large enough time value e.g., years
2. You make your fields *int where initialize the fields with the default value before applying options in protocol. Any value <0 is invalid (error). 0 would mean infinite (you remove the default and need to add nil check to protocol_lifecycle when reading the values).

Once finished with the PR please also update your description and link an issue

[nkreiger]

@embano1 Addressed all comments. I think the == 0 IMHO makes sense.

If the user wants an infinite timeout, they can still pass a negative value like -1, and it will be assigned correctly (see tests). Otherwise, it defaults to exactly how it behaved before, keeping backwards compatibility.

Let me know thoughts - updated and linked issue

[embano1]

@nkreiger thx!

The -1 behavior seems broken? Is that a valid value for a timeout? Never used that in http.Server, did you test it? For example time.Sleep() returns immediately on negative values.

Tbh, as raised earlier, I don't think we need the -1 case. Also, this is now a breaking change because we set a default timeout, right?

How about the following logic:

If With[Read|Write]Timeout is not specified, don't use a timeout, i.e., 0 (default value) means no timeout. The value must always be great than 0 in the option(s). This way we don't break existing users (unlimited), but allow specifying timeouts.

[nkreiger]

@nkreiger thx!

The -1 behavior seems broken? Is that a valid value for a timeout? Never used that in http.Server, did you test it? For example time.Sleep() returns immediately on negative values.

Tbh, as raised earlier, I don't think we need the -1 case. Also, this is now a breaking change because we set a default timeout, right?

How about the following logic:

If With[Read|Write]Timeout is not specified, don't use a timeout, i.e., 0 (default value) means no timeout. The value must always be great than 0 in the option(s). This way we don't break existing users (unlimited), but allow specifying timeouts.

@embano1 I'm okay with that, but I pulled this directly from the net/http documentation in the source code of Go.

	// ReadTimeout is the maximum duration for reading the entire
	// request, including the body. A zero or negative value means
	// there will be no timeout.
	//
	// Because ReadTimeout does not let Handlers make per-request
	// decisions on each request body's acceptable deadline or
	// upload rate, most users will prefer to use
	// ReadHeaderTimeout. It is valid to use them both.
	ReadTimeout time.Duration

// WriteTimeout 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 zero or negative value means there will be no timeout.
	WriteTimeout time.Duration

This is where I got the description as well, lol.

https://github.com/golang/go/blob/master/src/net/http/server.go#L2850

This is why I feel like it makes sense to allow this case

Add-On

Also, this should be non-breaking, the DefaultTimeout was already being set -- which is lead to the original issue, because I couldn't figure out why my server kept timing out in production :). This will keep that behavior, unless we don't want to do that, and default to no timeout.

[embano1]

Thanks for clarifying that a zero value is basically equal to 0. Tbh, from an SDK/ergonomics perspective, I'd rather keep it simple and not introduce multiple conditions where the user must understand/read what negative values mean. I don't see a benefit in allowing negative values (because that case is now covered with 0), but instead added cognitive load (as can be seen from this discussion and the fact that you had to look it up).

Our SDK should be easy to use so my suggestion is to allow >=0 timeouts where 0 is equivalent to the default behavior (no timeout). Please also indicate this in the field properties of the timeouts (0 == default == no timeout) and in the options (with additional checks to throw error on <0 values).

Thanks!

[embano1]

Also, this should be non-breaking, the DefaultTimeout was already being set -- which is lead to the original issue, because I couldn't figure out why my server kept timing out in production :). This will keep that behavior, unless we don't want to do that, and default to no timeout.

Got it! Thx, yes then it's non-breaking. However, now we need to distinguish between 0 and default field value again :) (I guess no way around *int then) if we want to support the infinite use case? Or don't we?

[nkreiger]

@embano1 Updated with the first part.

Did you want to distinguish between 0 and the default field with a pointer?

I feel like that might be a bit more confusing then just allowing negatives for infinite timeouts and documenting that?

Up to you, just let me know and I can make updates

[nkreiger]

@embano1 I've updated it with pointers so you can take a look and lmk what you think!

[embano1]

Thx! Based on your suggestion, here's my simplified proposal which:

• Uses 600s as default unless overwritten
• Can be overwritten only with values >=0
• 0 means infinite timeout

diff --git a/v2/protocol/http/options.go b/v2/protocol/http/options.go
index c77cab0..3590950 100644
--- a/v2/protocol/http/options.go
+++ b/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
diff --git a/v2/protocol/http/options_test.go b/v2/protocol/http/options_test.go
index 5647085..16a5da8 100644
--- a/v2/protocol/http/options_test.go
+++ b/v2/protocol/http/options_test.go
@@ -68,7 +68,7 @@ func TestWithTarget(t *testing.T) {
 			target: "%",
 			wantErr: "http target option failed to parse target url: " + func() string {
 				//lint:ignore SA1007 I want to create a bad url to test the error message
-				_, err := url.Parse("%") //nolint // I want to create a bad url to test the error message
+				_, err := url.Parse("%") // nolint // I want to create a bad url to test the error message
 				return err.Error()
 			}(),
 		},
@@ -92,7 +92,6 @@ func TestWithTarget(t *testing.T) {
 	}
 	for n, tc := range testCases {
 		t.Run(n, func(t *testing.T) {
-
 			err := tc.t.applyOptions(WithTarget(tc.target))
 
 			if tc.wantErr != "" || err != nil {
@@ -163,7 +162,6 @@ func TestWithMethod(t *testing.T) {
 	}
 	for n, tc := range testCases {
 		t.Run(n, func(t *testing.T) {
-
 			err := tc.t.applyOptions(WithMethod(tc.method))
 
 			if tc.wantErr != "" || err != nil {
@@ -247,7 +245,6 @@ func TestWithHeader(t *testing.T) {
 	}
 	for n, tc := range testCases {
 		t.Run(n, func(t *testing.T) {
-
 			err := tc.t.applyOptions(WithHeader(tc.key, tc.value))
 
 			if tc.wantErr != "" || err != nil {
@@ -291,7 +288,6 @@ func TestWithShutdownTimeout(t *testing.T) {
 	}
 	for n, tc := range testCases {
 		t.Run(n, func(t *testing.T) {
-
 			err := tc.t.applyOptions(WithShutdownTimeout(tc.timeout))
 
 			if tc.wantErr != "" || err != nil {
@@ -333,7 +329,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",
@@ -341,7 +337,6 @@ func TestWithReadTimeout(t *testing.T) {
 	}
 	for n, tc := range testCases {
 		t.Run(n, func(t *testing.T) {
-
 			err := tc.t.applyOptions(WithReadTimeout(tc.timeout))
 
 			if tc.wantErr != "" || err != nil {
@@ -367,7 +362,6 @@ func TestWithReadTimeout(t *testing.T) {
 
 func TestWithWriteTimeout(t *testing.T) {
 	expected := time.Minute * 4
-
 	testCases := map[string]struct {
 		t       *Protocol
 		timeout time.Duration
@@ -384,7 +378,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",
@@ -392,7 +386,6 @@ func TestWithWriteTimeout(t *testing.T) {
 	}
 	for n, tc := range testCases {
 		t.Run(n, func(t *testing.T) {
-
 			err := tc.t.applyOptions(WithWriteTimeout(tc.timeout))
 
 			if tc.wantErr != "" || err != nil {
@@ -415,6 +408,7 @@ func TestWithWriteTimeout(t *testing.T) {
 		})
 	}
 }
+
 func TestWithPort(t *testing.T) {
 	testCases := map[string]struct {
 		t       *Protocol
@@ -457,7 +451,6 @@ func TestWithPort(t *testing.T) {
 	}
 	for n, tc := range testCases {
 		t.Run(n, func(t *testing.T) {
-
 			err := tc.t.applyOptions(WithPort(tc.port))
 
 			if tc.wantErr != "" || err != nil {
@@ -489,9 +482,17 @@ 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 },
+		"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) {
@@ -569,7 +570,6 @@ func TestWithListener(t *testing.T) {
 	}
 	for n, tc := range testCases {
 		t.Run(n, func(t *testing.T) {
-
 			err := tc.t.applyOptions(WithListener(tc.listener))
 
 			if tc.wantErr != "" || err != nil {
@@ -618,7 +618,6 @@ func TestWithPath(t *testing.T) {
 	}
 	for n, tc := range testCases {
 		t.Run(n, func(t *testing.T) {
-
 			err := tc.t.applyOptions(WithPath(tc.path))
 
 			if tc.wantErr != "" || err != nil {
diff --git a/v2/protocol/http/protocol.go b/v2/protocol/http/protocol.go
index 4bb07c4..18bd604 100644
--- a/v2/protocol/http/protocol.go
+++ b/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 {
diff --git a/v2/protocol/http/protocol_lifecycle.go b/v2/protocol/http/protocol_lifecycle.go
index 4fc39ae..7551c31 100644
--- a/v2/protocol/http/protocol_lifecycle.go
+++ b/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,
 	}

[nkreiger]

@embano1 made all the changes, let me know what you think!

[embano1]

Great, thank you! Looks good! Please make sure to rebase on the latest commit (if needed) and squash your commits into with a nice commit message :)

[nkreiger]

@embano1 hate to bother you once more, the git rebase seems to be the only command I can't get to run successfully, ever. It always errors out, any suggestions on a command sequence?

I tried

git rebase -i HEAD~51 

then changing pick -> squash

If I try to autosquash I just get continuous merge conflicts in the test/conformance/go.mod

[nkreiger]

@embano1 I messed it up, then redid it by resetting back to the main branch, and merging a branch of the code into my forked branch with "Squash and Merge".

[embano1]

Thank you! Just checking if you want this commit title and message to be the final one? Says backup and 11 authors right now. Might be confusing for those looking at it at a later point since the commit title/message does not align with your PR.

Perhaps this helps to fix? https://www.mgasch.com/2021/05/git-basics/

[nkreiger]

@embano1 not having much luck. Where do you see that title? the PR title looks good to me from my side.

I'm good with this if its alright, I'm hoping to fix this bug in production as soon as possible, and move off the forked private package.

[embano1]

@embano1 not having much luck. Where do you see that title? the PR title looks good to me from my side.

Click on Commits tab:

I'm good with this if its alright, I'm hoping to fix this bug in production as soon as possible, and move off the forked private package.

We generally prefer clean commit messages as per our CONTRIBUTING guidelines to ease debugging/troubleshooting for our later selfs, but I understand that Git can be challenging.

Please try this from within your PR branch:

git fetch --all

# unstage all changes - replace origin with whatever you use as the remote repo for cloudevents
git reset origin/main

# add your changes
git add v2/protocol/http

# create clean commit
git commit -s -m "feat: make http timeout configurable"

# push to your fork
git push fork --force-with-lease

I can see that the issues you faced where because you were using main instead of a proper branch. For future commits, Please take a look at https://www.mgasch.com/2021/05/git-basics/ for some git best practices/contributing flows.

[nkreiger]

@embano1 ran through my steps again, this time I cleaned up the commits when I squashed it. Looking much better on my side, if you can confirm on yours! Thanks.

Sorry, jumping in between projects, and I forked this as a private repository before moving it back to PR it in, so I jumped through some git hoops I don't usually do. My fault, apologies for any time wasted and I'll make sure I re-read the guidelines before making another contribution!

[embano1]

Great work!