Retry only on real network errors #1549

m3co-code · 2017-05-04T10:09:44Z

Now retries only happen when actual network errors occur and not only
anymore based on the HTTP status code. This is because the backend could
also send this status codes as their normal interface and in that case
we don't want to retry.

This PR should solve the issue 1104

timoreimann

Left some comments. :-)

timoreimann · 2017-05-04T10:50:46Z

middlewares/retry.go

@@ -42,10 +43,16 @@ func (retry *Retry) ServeHTTP(rw http.ResponseWriter, r *http.Request) {
 	}
 	attempts := 1
 	for {
+		netErrorOccured := false


Typo: occurred

In the comment as well.

Thanks, fixed.

timoreimann · 2017-05-04T10:51:50Z

middlewares/retry.go

@@ -42,10 +43,16 @@ func (retry *Retry) ServeHTTP(rw http.ResponseWriter, r *http.Request) {
 	}
 	attempts := 1
 	for {
+		netErrorOccured := false
+		// We pass in a pointer to netErrorOccured so that we can set it to true on network errors
+		// that happen when proxying the http requests to the backends.


Maybe mention that it's a custom error handler which will set the flag?

Ok, I extended the comment.

timoreimann · 2017-05-04T10:52:31Z

middlewares/retry.go

-		if !isNetworkError(recorder.Code) || attempts >= retry.attempts {
+
+		retry.next.ServeHTTP(recorder, r.WithContext(newCtx))
+		if netErrorOccured == false || attempts >= retry.attempts {


Simplify: !netErrorOccurred

timoreimann · 2017-05-04T10:53:20Z

middlewares/retry.go

-	return status == http.StatusBadGateway || status == http.StatusGatewayTimeout
-}
+// NetErrorCtxKey is a custom type that is used as key for the request context.
+// Using an own type we avoid potential collisions with other keys that may have the same string name.


Nice, didn't know that was possible! 🎉

Actually its even a best practice to not export the type if it is not necessary, to have real safety. Though I decided to have the error handler in the server package still, as for me it makes more sense there, which requires to export the type.

I refactored the implementation now, so that the knowledge about how to interact with the context completely to the retry middleware and therefore even can use a private type.

timoreimann · 2017-05-04T16:40:35Z

middlewares/retry_test.go

+			httpHandler = NewRetry(tc.retryCount, httpHandler)
+
+			recorder := httptest.NewRecorder()
+			req, err := http.NewRequest("GET", "http://localhost:3000/ok", ioutil.NopCloser(nil))


you could s/"GET"/http.MethodGet/ here.

In some other parts of the PR too.

Thanks, fixed at all places.

timoreimann · 2017-05-04T16:54:26Z

server/errorhandler.go

+func recordNetError(req *http.Request) {
+	val := req.Context().Value(middlewares.DefaultNetErrCtxKey)
+
+	if netErrorOccured, ok := val.(*bool); ok {


Does this explode if val is nil (i.e., the value does not exist), or will Go handle this fine with ok being false?

The ctx.Value() guarantees a return value of type interface{}, so we can do a type assertion without danger.

timoreimann · 2017-05-04T16:55:06Z

middlewares/retry_test.go

+
+	for _, failAtCall := range handler.failAtCalls {
+		if handler.callNumber == failAtCall {
+			val := r.Context().Value(DefaultNetErrCtxKey)


How about exporting server.recordNetError and calling it here?

As said before, I refactored this implementation, so the implementation for recording the network error is now in the middleware package and the tests make more sense.

timoreimann · 2017-05-04T16:57:10Z

server/errorhandler_test.go

+			netErrorOccured := false
+			req, err := http.NewRequest("GET", "http://localhost:3000/any", nil)
+			if err != nil {
+				panic(fmt.Sprintf("NewRequest(): %s", err))


A MustNewRequest helper function might become convenient at this point.

There is an util function in httptest.NewRequest that will internally panic if the creation of the NewRequest fails. I think this would be good to use for the tests.

timoreimann · 2017-05-04T16:59:13Z

server/errorhandler.go

+// RecordingErrorHandler is an error handler, implementing the vulcand/oxy
+// error handler interface, which is recording network errors by setting the
+// request context value with the key middlewares.DefaultNetErrCtxKey to true.
+// This enables the retry middleware to only retry on actual network errors.


IMHO we should also document the type of errors it maps to 502 and 504, respectively.

I added a general comment that it will map to the proper status codes. I fear that being to explicit in the comment will bring up the danger, that the implementation and the comment will drift apart at some time. And when people read the comment, they probably also can read the code.

timoreimann · 2017-05-04T17:04:14Z

server/errorhandler_test.go

+			err:                errors.New("any error"),
+			wantHTTPStatus:     http.StatusInternalServerError,
+			wantNetErrRecorded: false,
+		},


Are we missing a test case for err == nil?

Yes, better to also add a test for this. Doing.

gottwald · 2017-05-05T07:33:00Z

middlewares/retry.go

-}
+// NetErrorCtxKey is a custom type that is used as key for the request context.
+// Using an own type we avoid potential collisions with other keys that may have the same string name.
+type NetErrorCtxKey string


From my learnings about context I remember that the key should be a custom type as you do here, but it should also be non exported, since it's something that should be package internal.
Obviously this is not straightforward here because the server package needs to signal something to the middleware package.
But instead of exporting low level package stuff, should the retry middlewares package maybe export some functions to deal with this and keep the context key type private?
Looks like you already have some kind of function in the server package called: recordNetError.
This should probably be moved to the middlewares package.
Also recordNetError doesn't need to get passed a request, a context would be sufficient.

An internal package might be helpful here too.

Thanks for the hint, I refactored it now in this way. Now the logic for mutating the context is solely in the retry middleware and therefore we can use a private type 🎉

@timoreimann I am not sure how this should look like to be honest. Also I am hesitant to change the middleware structure in this PR, as imho it would be separate concern. Anyway I think for a long term it could be beneficial to bring more structure into the middleware package.

m3co-code · 2017-05-05T09:06:35Z

@timoreimann and @gottwald thanks for the review, I worked your feedback into the PR.

m3co-code · 2017-05-05T09:13:50Z

middlewares/retry_test.go

+}
+
+func TestRecordNetError(t *testing.T) {
+	boolNetErrorOccurred := false


A small hint for reviewers, I tried to build this as a table driven test at first, but I was not able to get it working in a straightforward way, as it would require me to specify one type for the initial context value I am passing in. This would have to be an interface{} in my case, but then the type assertion on a *bool in the DefaultRecordNetError does not work anymore.

Then let's have three separate test functions so that we can easily see which test failed, can easily rerun them, etc.

timoreimann

Some smallish things left.

timoreimann · 2017-05-05T09:15:43Z

middlewares/retry.go

+
+// NetErrorRecorder is an interface to record net errors.
+type NetErrorRecorder interface {
+	// RecordNetError can be used to signal the retry middleware, that an network error happened


No comma before that.

Thanks, done.

timoreimann · 2017-05-05T09:24:04Z

middlewares/retry_test.go

+}
+
+func TestRecordNetError(t *testing.T) {
+	boolNetErrorOccurred := false


Then let's have three separate test functions so that we can easily see which test failed, can easily rerun them, etc.

timoreimann · 2017-05-05T09:28:27Z

middlewares/retry.go

@@ -42,10 +43,16 @@ func (retry *Retry) ServeHTTP(rw http.ResponseWriter, r *http.Request) {
 	}
 	attempts := 1
 	for {
+		netErrorOccurred := false
+		// We pass in a pointer to netErrorOccurred so that we can set it to true on network errors
+		// when proxying the http requests to the backends. This happens in the custom RecordingErrorHandler.


s/http/HTTP/

Thanks, done.

timoreimann · 2017-05-05T09:28:30Z

server/errorhandler.go

+
+// RecordingErrorHandler is an error handler, implementing the vulcand/oxy
+// error handler interface, which is recording network errors by using the netErrorRecorder.
+// In addition it sets a proper http status code and body, depending on the type of error occurred.


s/http/HTTP/

Thanks, done.

m3co-code · 2017-05-05T09:38:47Z

Ok, worked in feedback again.

timoreimann

LGTM. ☺️

gottwald · 2017-05-05T20:46:41Z

server/errorhandler.go

+	statusCode := http.StatusInternalServerError
+
+	if e, ok := err.(net.Error); ok {
+		eh.netErrorRecorder.RecordNetError(req.Context())


nit-pick: This stutters a bit, maybe call the method Record instead?

Thanks for pointing this out, changed it.

m3co-code · 2017-05-13T16:37:23Z

@emilevauge or @vdemeester can you have a look?

ldez · 2017-05-15T08:55:06Z

Due to SemaphoreCI, I close and reopen the PR.

emilevauge

Hey @marco-jantke, thanks for your contribution :)
I made few comments, but all in all, seems good !

emilevauge · 2017-05-18T16:08:28Z

middlewares/retry.go

+}
+
+// NetErrorRecorderFunc is an util to be able to implement the NetErrorRecorder without the need of a struct.
+type NetErrorRecorderFunc func(ctx context.Context)


What is the need of doing this ? Why don't you want to use a struct here ?

The implementation has no need of any field. I originally had a struct here without fields, but got a hint in a previous review here that a function would suffice. Therefore I added this type in order to able to implement the NetErrorRecorder interface on a function.

This said I am fine with both implementations. An empty struct is a bit less code, but in some way unnecessary. As you prefer :)

If that's OK for you, I would prefer an empty struct, which is easier to read, and with the same result :)

No problem, I changed it to a struct now. See 6e291db. Though to add good documentation to the struct though, its a bit like Captain Obvious, but adhering to the linting :)

emilevauge · 2017-05-18T16:10:39Z

middlewares/retry.go

+func DefaultRecordNetError(ctx context.Context) {
+	val := ctx.Value(defaultNetErrCtxKey)
+
+	if netErrorOccurred, ok := val.(*bool); ok {


Not easy to read.

I renamed ok now to isBoolPointer. Not sure how to improve readability in another way.

emilevauge · 2017-05-18T16:15:57Z

server/errorhandler.go

+
+	"github.com/containous/traefik/middlewares"
+)
+


You will probably get issues with websockets here.
You should add

var ( _ Stateful = &RecordingErrorHandler{} )

Hmm it has none of the capabilities the Stateful interface expects. Maybe the naming is rather not perfect then? It is not an extension of ResponseWriter or whatsoever, its an implementation of the oxy ErrorHandler interface. Its called Recording because it calls the NetErrorRecorder which in our case will manipulate the request context. So I think this is unnecessary here. Any suggestion for naming improvements?

You're right, successfully tested your code with websocket 👍

m3co-code · 2017-05-19T10:07:50Z

@emilevauge please have a look again, incorporated/answered your questions.

emilevauge · 2017-05-19T13:46:13Z

server/errorhandler.go

+		} else {
+			statusCode = http.StatusBadGateway
+		}
+	} else if err == io.EOF {


Reading this part again, not sure this is the right way to go. We could get an EOF error not due to network right ?

About this one I am not 100% sure whether its right to retry here. I am currently trying to investigate use-cases when an io.EOF can happen. For sure it can be due to a network connection problem, but also when the server closes the connection by intend. Maybe the server would do so because of too much load and we should rather try another one? Anyway I think there is no way to vary in between those cases and thats why in general I would rather also retry those request.

@emilevauge WDYT?

@timoreimann any thoughts on this ?

@emilevauge @marco-jantke the net package does distinguish between an an ErrUnexpectedEOF and EOF state. Unfortunately, it's not used by net/http.

Even if we considered this to be changeworthy, IMHO we should not do so along this PR which maintains the current oxy error handler behavior except for the response code logic. We can have a separate issue to drive a potential improvement in this focused regard.

@marco-jantke I agree with @timoreimann on this, we should deal with this issue in another PR. The fact that it is not used in the default go http client leads to discussions :)

I agree, so we handled all the questions/feedback in this PR I think :)

m3co-code · 2017-05-29T07:14:05Z

@timoreimann @emilevauge you are ok with me doing a rebase already to resolve the conflicts?

timoreimann · 2017-05-29T07:35:20Z

@marco-jantke I think it's okay to do that now.

m3co-code · 2017-05-30T13:43:16Z

Ok, everything should be ready now.

m3co-code · 2017-06-06T10:16:42Z

@emilevauge ping :)

ldez · 2017-06-07T00:54:49Z

@marco-jantke We browse all PRs regularly, you don't need to ping someone.

emilevauge

Thanks @marco-jantke !
LGTM

Now retries only happen when actual network errors occur and not only anymore based on the HTTP status code. This is because the backend could also send this status codes as their normal interface and in that case we don't want to retry.

ldez added the kind/enhancement a new or improved feature. label May 4, 2017

timoreimann suggested changes May 4, 2017

View reviewed changes

gottwald reviewed May 5, 2017

View reviewed changes

m3co-code commented May 5, 2017

View reviewed changes

timoreimann suggested changes May 5, 2017

View reviewed changes

timoreimann approved these changes May 5, 2017

View reviewed changes

gottwald reviewed May 5, 2017

View reviewed changes

timoreimann mentioned this pull request May 9, 2017

Custom error page feature #1576

Closed

timoreimann added the size/M label May 10, 2017

ldez added the status/2-needs-review label May 12, 2017

m3co-code force-pushed the 1104-retry-behaviour branch 2 times, most recently from 6b413f7 to 5a0d6ee Compare May 12, 2017 08:00

ldez closed this May 15, 2017

ldez reopened this May 15, 2017

emilevauge requested changes May 18, 2017

View reviewed changes

emilevauge reviewed May 19, 2017

View reviewed changes

emilevauge added the contributor/waiting-for-corrections label May 22, 2017

m3co-code force-pushed the 1104-retry-behaviour branch 2 times, most recently from fd565f7 to 70df8ab Compare May 29, 2017 13:34

ldez removed the contributor/waiting-for-corrections label Jun 3, 2017

m3co-code force-pushed the 1104-retry-behaviour branch from 70df8ab to 1d0b4ba Compare June 6, 2017 10:18

m3co-code force-pushed the 1104-retry-behaviour branch 4 times, most recently from 8154f75 to 3b3b046 Compare June 13, 2017 13:14

m3co-code force-pushed the 1104-retry-behaviour branch from 3b3b046 to a04d509 Compare June 15, 2017 09:04

emilevauge approved these changes Jun 19, 2017

View reviewed changes

emilevauge added status/3-needs-merge and removed status/2-needs-review labels Jun 19, 2017

retry only on real network errors

9f17ce1

Now retries only happen when actual network errors occur and not only anymore based on the HTTP status code. This is because the backend could also send this status codes as their normal interface and in that case we don't want to retry.

ldez force-pushed the 1104-retry-behaviour branch from a04d509 to 9f17ce1 Compare June 19, 2017 17:56

ldez removed the status/3-needs-merge label Jun 19, 2017

ldez merged commit f79317a into traefik:master Jun 19, 2017

ldez mentioned this pull request Jun 19, 2017

retry behaviour #1104

Closed

m3co-code deleted the 1104-retry-behaviour branch June 20, 2017 06:47

ldez modified the milestone: 1.4 Jun 20, 2017

ldez added the area/middleware label Aug 14, 2017

ldez changed the title ~~retry only on real network errors~~ Retry only on real network errors Aug 14, 2017

Retry only on real network errors #1549

Retry only on real network errors #1549

Conversation

m3co-code commented May 4, 2017

timoreimann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m3co-code May 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m3co-code May 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m3co-code May 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timoreimann May 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m3co-code commented May 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timoreimann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m3co-code commented May 5, 2017

timoreimann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m3co-code commented May 13, 2017

ldez commented May 15, 2017

emilevauge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m3co-code May 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m3co-code commented May 19, 2017

emilevauge May 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m3co-code commented May 29, 2017

timoreimann commented May 29, 2017

m3co-code commented May 30, 2017

m3co-code May 5, 2017 •

edited

Loading

m3co-code May 5, 2017 •

edited

Loading

m3co-code May 5, 2017 •

edited

Loading

timoreimann May 5, 2017 •

edited

Loading

m3co-code May 19, 2017 •

edited

Loading

emilevauge May 19, 2017 •

edited

Loading