feat: add recovery middleware to handle panic gracefully#1537
Conversation
|
I agree that adding a recovery layer makes sense. gin.New() does not include one by default, so today a panic can indeed end up as a dropped connection instead of a controlled response. That said, I do not think this implementation is quite correct yet, because it assumes every panic can be turned into the same JSON 500 response. There are at least two cases where that is not very elegant:
So I think the underlying concern is valid, but the current fix is a bit too broad. A more robust approach would be to either:
In short: the direction is reasonable, but handling all panics with the same AbortWithStatusJSON(500, ...) is not especially elegant for SSE and HTML paths. |
Fixes #1536
Proposed Changes
middleware.Recovery()that catches panics from any subsequent middleware or handler, logs the panic message with full stack trace vialog.Errorf+debug.Stack(), and returns a unified 500 JSON response (reason: base.unknown) viahandler.NewRespBody+TrMsg— consistent with how other errors are handled ininternal/base/handler/handler.go.Recovery()as the first middleware ininternal/base/server/http.goso it covers all subsequent middleware (brotli, accept-language, short-id, auth, etc.) and handlers.recovery_test.gocovering both the panic path (verifies 500 +base.unknownreason) and the no-panic path (verifies normal requests pass through unaffected).