gin-gonic / gin

Gin is a HTTP web framework written in Go (Golang). It features a Martini-like API with much better performance -- up to 40 times faster. If you need smashing performance, get yourself some Gin.
https://gin-gonic.com/
MIT License
77.95k stars 7.97k forks source link

RecoveryWithWriter takes mulitple RecoveryFuncs but only applies the first #4044

Open nesselchen opened 2 weeks ago

nesselchen commented 2 weeks ago

Description

RecoveryWithWritter has signature func(w io.Writer, recovery ...gin.RecoveryFunc) but only applies the first func from recovery. If no RecoveryFuncs were passed, it uses the defaultHandleRecovery func.

If I want to register multiple RecoveryFuncs, it would seem plausible to apply them all at once, but with this implementation most would get discarded silently. Additionally, this results in a panicking request returning code 200 instead of the pre-configured code 500.

I don't see why it couldn't allow multiple RecoveryFuncs with each handling the error separately. Alternatively I would appreciate it if the documentation gave hints to this being the intended behavior.

How to reproduce

Here's the excerpt from the gin package (recover.go:43)

// RecoveryWithWriter returns a middleware for a given writer that recovers from any panics and writes a 500 if there was one.
func RecoveryWithWriter(out io.Writer, recovery ...RecoveryFunc) HandlerFunc {
    if len(recovery) > 0 {
        return CustomRecoveryWithWriter(out, recovery[0])
    }
    return CustomRecoveryWithWriter(out, defaultHandleRecovery)
}

Expectations


func PrintErr(_ *gin.Context, err any) {
  if err != nil {
    fmt.Println(err)
  }
}

func Abort(ctx *gin.Context, _ any) {
  ctx.AbortWithStatus(500)
}

// ...

router.Use(RecoveryWithWriter(logger, PrintErr, Abort)) // -> should print the err and abort the request

## Actual result

Under the current implementation only the error would get logged, with the request still returning 200 after recovering.

## Environment

- go version: go1.22.6
- gin version (or commit ref):  github.com/gin-gonic/gin v1.9.1
- operating system: macOS
JimChenWYU commented 2 weeks ago

I think it's not support mulitple RecoveryFuncs but provides optional parameters.

nesselchen commented 2 weeks ago

Yeah, that's what I assumed. Couldn't you put that in the documentation to limit the risk? Either that or changing it to allow multiple RecoveryFuncs. I still don't really see why since they don't interfere with each other.

RedCrazyGhost commented 1 week ago

https://github.com/gin-gonic/gin/commit/4cabdd303fe38b6b53e83a6aa04d0468a71c0139 The last submission has been 4 years ago, and this part may involve problems with the gin execution stack, which has not been processed