Fix logging aborts as error in retry middleware

[kianelbo]

What does this PR do?

This PR logs http.ErrAbortHandler in retry middleware as 'debug' instead of 'error'.

Motivation

Closes #9188

More

• [N/A] Added/updated tests
• [N/A] Added/updated documentation

[kevinpollet]

Hello @kianelbo,

Thanks for your interest in Traefik,

This PR is labeled as needs-design-review, as you can see in our contribution guide, this means that we have to take a longer look to evaluate how it would interact with the other parts of Traefik.

We will come back to you once the design review iteration is done.

[mpl]

@kianelbo you couldn't know about this, but this approach (capturing the panic in the retry middleware) isn't ideal because it makes the recovery middleware useless if/when the retry middleware is used. Consider this simplified view of how traefik works:

package main

import (
	"fmt"
)

func recoverFoo() {
	if err := recover(); err != nil {
		fmt.Printf("RECOVERED FROM FOO: %v\n", err)
	}
}

func retryMiddleware(work func()) {
	defer recoverFoo()
	work()
}

func recoverBar() {
	if err := recover(); err != nil {
		fmt.Printf("RECOVERED FROM BAR: %v\n", err)
	}
}

func recoverMiddleware(work func()) {
	defer recoverBar()
	work()
}

func routerHandler() {
	panic("IN WORK")
}

func main() {
	// retryMiddleware is the retry middleware, chained directly to the router handler.
	// recoverMiddleware is the recovery middleware, chained to the muxer of all routers.
	// -> if retryMiddleware recovers from the panic in routerHandler,
	// recoverMiddleware never gets the panic -> recovery becomes useless.
	recoverMiddleware(func() {retryMiddleware(routerHandler)})
}

I guess one could argue that the retry middleware could replicate exactly everything that the recovery middleware does, which overall would maybe keep things behaving as they should, but it sounds like a bad design to have the recovery done in two different places imho.

So I would suggest trying to fix the problem directly in the recovery middleware instead. But I haven't tried myself.

[kianelbo]

@mpl I totally agree with you. However in the issue it was said that the retry middleware should log aborts with debug level (just like recovery) and that's what I did.

So I would suggest trying to fix the problem directly in the recovery middleware instead. But I haven't tried myself.

Handling panic in retry was an existing behavior. So what do you suggest? Adding retry to recovery middleware or ...? :)

[ldez]

I suggest not retrying on this error and just catching the error inside the recovery middleware.

[kianelbo]

I suggest not retrying on this error and just catching the error inside the recovery middleware.

@ldez so you mean removing the recovery routine inside retry middleware (because it should be handled with recovery middleware) and also stop retrying on aborts? If so how is it possible to detect the abort in retry without a recover()?

[ldez]

Hello,

in fact, the panics must not be caught, even the http.ErrAbortHandler, because the response state will be completely unusable. The recovery middleware will catch and handle it in the right way.

Close in favor of #9284.