You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@answer.apache.org by "hhc7 (via GitHub)" <gi...@apache.org> on 2026/05/28 10:25:38 UTC
[PR] feat: add recovery middleware to handle panic gracefully [answer]
hhc7 opened a new pull request, #1537:
URL: https://github.com/apache/answer/pull/1537
Fixes #1536
## Proposed Changes
- Add `middleware.Recovery()` that catches panics from any subsequent middleware or handler, logs the panic message with full stack trace via `log.Errorf` + `debug.Stack()`, and returns a unified 500 JSON response (`reason: base.unknown`) via `handler.NewRespBody` + `TrMsg` — consistent with how other errors are handled in `internal/base/handler/handler.go`.
- Mount `Recovery()` as the first middleware in `internal/base/server/http.go` so it covers all subsequent middleware (brotli, accept-language, short-id, auth, etc.) and handlers.
- Add unit tests in `recovery_test.go` covering both the panic path (verifies 500 + `base.unknown` reason) and the no-panic path (verifies normal requests pass through unaffected).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@answer.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@answer.apache.org
For additional commands, e-mail: issues-help@answer.apache.org
Re: [PR] feat: add recovery middleware to handle panic gracefully [answer]
Posted by "hhc7 (via GitHub)" <gi...@apache.org>.
hhc7 commented on PR #1537:
URL: https://github.com/apache/answer/pull/1537#issuecomment-4598898363
Thanks for the thoughtful review. I'll rework the middleware along the response-aware approach (your second suggestion):
1. Check ctx.Writer.Written() first. If true (SSE / already-flushed responses), only log + abort, never try to write a body.
2. Otherwise, branch by route family — JSON 500 for API routes (\*/api/\*), and AbortWithStatus(500) for HTML routes so the browser shows its native error page rather than raw JSON.
Keeping recovery mounted at the engine level guarantees no future route group accidentally loses panic coverage. Will push the update shortly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@answer.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@answer.apache.org
For additional commands, e-mail: issues-help@answer.apache.org
Re: [PR] feat: add recovery middleware to handle panic gracefully [answer]
Posted by "LinkinStars (via GitHub)" <gi...@apache.org>.
LinkinStars commented on PR #1537:
URL: https://github.com/apache/answer/pull/1537#issuecomment-4581900983
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:
1. SSE / streaming endpoints
For /chat/completions, the handler starts an SSE response very early: it sets Content-Type: text/event-stream, sends status 200, flushes headers, and may already write streamed chunks before later logic runs. If a panic happens after that point,
recovery cannot reliably convert the response into a JSON 500. In practice it may just append JSON into an already-started event stream, which produces an invalid/mixed response rather than a clean error.
2. HTML routes
This server also handles UI / HTML routes, not only JSON APIs. With a global recovery mounted at the engine level, a panic in an HTML route would now also return the API-style JSON body. That is probably not the intended behavior for page requests,
and it is not really “consistent” for the whole server, only for JSON endpoints.
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:
- scope JSON recovery only to API routes, or
- make recovery response-aware, for example checking whether headers/body were already written and whether the request is an API, SSE, or HTML route.
In short: the direction is reasonable, but handling all panics with the same AbortWithStatusJSON(500, ...) is not especially elegant for SSE and HTML paths.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@answer.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@answer.apache.org
For additional commands, e-mail: issues-help@answer.apache.org
Re: [PR] feat: add recovery middleware to handle panic gracefully [answer]
Posted by "LinkinStars (via GitHub)" <gi...@apache.org>.
LinkinStars merged PR #1537:
URL: https://github.com/apache/answer/pull/1537
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@answer.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@answer.apache.org
For additional commands, e-mail: issues-help@answer.apache.org