Setting ENABLE_GZIP=true makes swagger API URL 500 (NPE panic) #19839
Labels
issue/confirmed
Issue has been reviewed and confirmed to be present or accepted to be implemented
type/bug
Milestone
Comments
wxiaoguang
added
type/bug
issue/confirmed
|
The next log line tells you where/which handler is causing this: |
|
Now if I add some tracing into web wrap I see: This double context.Auth check is interesting... |
|
In fact it gets more interesting if we add some logging in to check the done status within that function Patchdiff --git a/modules/context/context.go b/modules/context/context.go
index dcc43973c..d851ac875 100644
--- a/modules/context/context.go
+++ b/modules/context/context.go
@@ -230,6 +230,7 @@ func (ctx *Context) HTML(status int, name base.TplName) {
}
ctx.ServerError("Render failed", err)
}
+ ctx.Resp.Flush()
}
// RenderToString renders the template content to a string
diff --git a/modules/web/wrap_convert.go b/modules/web/wrap_convert.go
index 8dc4e6d62..42e203263 100644
--- a/modules/web/wrap_convert.go
+++ b/modules/web/wrap_convert.go
@@ -10,6 +10,7 @@ import (
"net/http"
"code.gitea.io/gitea/modules/context"
+ "code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/web/routing"
)
@@ -17,72 +18,90 @@ type wrappedHandlerFunc func(resp http.ResponseWriter, req *http.Request, others
func convertHandler(handler interface{}) wrappedHandlerFunc {
funcInfo := routing.GetFuncInfo(handler)
+
switch t := handler.(type) {
case http.HandlerFunc:
return func(resp http.ResponseWriter, req *http.Request, others ...wrappedHandlerFunc) (done bool, deferrable func()) {
routing.UpdateFuncInfo(req.Context(), funcInfo)
+ log.Info("%v", funcInfo)
t(resp, req)
if r, ok := resp.(context.ResponseWriter); ok && r.Status() > 0 {
done = true
}
+ log.Info("%v:%t", funcInfo, done)
return
}
case func(http.ResponseWriter, *http.Request):
return func(resp http.ResponseWriter, req *http.Request, others ...wrappedHandlerFunc) (done bool, deferrable func()) {
routing.UpdateFuncInfo(req.Context(), funcInfo)
+ log.Info("%v", funcInfo)
t(resp, req)
if r, ok := resp.(context.ResponseWriter); ok && r.Status() > 0 {
done = true
}
+ log.Info("%v:%t", funcInfo, done)
return
}
case func(ctx *context.Context):
return func(resp http.ResponseWriter, req *http.Request, others ...wrappedHandlerFunc) (done bool, deferrable func()) {
routing.UpdateFuncInfo(req.Context(), funcInfo)
+ log.Info("%v", funcInfo)
ctx := context.GetContext(req)
t(ctx)
done = ctx.Written()
+ log.Info("%v:%t", funcInfo, done)
return
}
case func(ctx *context.Context) goctx.CancelFunc:
return func(resp http.ResponseWriter, req *http.Request, others ...wrappedHandlerFunc) (done bool, deferrable func()) {
routing.UpdateFuncInfo(req.Context(), funcInfo)
+ log.Info("%v", funcInfo)
ctx := context.GetContext(req)
deferrable = t(ctx)
done = ctx.Written()
+ log.Info("%v:%t", funcInfo, done)
return
}
case func(*context.APIContext):
return func(resp http.ResponseWriter, req *http.Request, others ...wrappedHandlerFunc) (done bool, deferrable func()) {
routing.UpdateFuncInfo(req.Context(), funcInfo)
+ log.Info("%v", funcInfo)
ctx := context.GetAPIContext(req)
t(ctx)
done = ctx.Written()
+ log.Info("%v:%t", funcInfo, done)
return
}
case func(*context.APIContext) goctx.CancelFunc:
return func(resp http.ResponseWriter, req *http.Request, others ...wrappedHandlerFunc) (done bool, deferrable func()) {
routing.UpdateFuncInfo(req.Context(), funcInfo)
+ log.Info("%v", funcInfo)
ctx := context.GetAPIContext(req)
deferrable = t(ctx)
done = ctx.Written()
+ log.Info("%v:%t", funcInfo, done)
return
}
case func(*context.PrivateContext):
return func(resp http.ResponseWriter, req *http.Request, others ...wrappedHandlerFunc) (done bool, deferrable func()) {
routing.UpdateFuncInfo(req.Context(), funcInfo)
+ log.Info("%v", funcInfo)
ctx := context.GetPrivateContext(req)
t(ctx)
done = ctx.Written()
+ log.Info("%v:%t", funcInfo, done)
return
}
case func(*context.PrivateContext) goctx.CancelFunc:
return func(resp http.ResponseWriter, req *http.Request, others ...wrappedHandlerFunc) (done bool, deferrable func()) {
routing.UpdateFuncInfo(req.Context(), funcInfo)
+ log.Info("%v", funcInfo)
ctx := context.GetPrivateContext(req)
deferrable = t(ctx)
done = ctx.Written()
+ log.Info("%v:%t", funcInfo, done)
+
return
}
case func(http.Handler) http.Handler:
@@ -92,10 +111,13 @@ func convertHandler(handler interface{}) wrappedHandlerFunc {
next = wrapInternal(others)
}
routing.UpdateFuncInfo(req.Context(), funcInfo)
+ log.Info("%v", funcInfo)
t(next).ServeHTTP(resp, req)
if r, ok := resp.(context.ResponseWriter); ok && r.Status() > 0 {
done = true
}
+ log.Info("%v:%t", funcInfo, done)
+
return
}
default:
Produces logs like this: Now the interesting thing lines is Here this is saying that the response wasn't written to. But it should have been ... And if we look a bit higher we see: if r, ok := resp.(context.ResponseWriter); ok && r.Status() > 0 {
done = true
}But the resp that we are passed here is a So this done never gets set. We need to wrap the resp if it's not a ResponseWriter |
zeripath
added a commit
to zeripath/gitea
that referenced
this issue
May 30, 2022
In order for web.Wrap to be able to detect if a response has been written we need to wrap any non-context.ResponseWriters as a such. Otherwise responses will be incorrectly detected as non-written to and handlers can double run. In the case of GZip this handler will change the response to a non-context.RW and this failure to correctly detect response writing causes fallthrough and a NPE. Fix go-gitea#19839 Signed-off-by: Andrew Thornton <art27@cantab.net>
zeripath
added a commit
that referenced
this issue
May 31, 2022
In order for web.Wrap to be able to detect if a response has been written we need to wrap any non-context.ResponseWriters as a such. Otherwise responses will be incorrectly detected as non-written to and handlers can double run. In the case of GZip this handler will change the response to a non-context.RW and this failure to correctly detect response writing causes fallthrough and a NPE. Fix #19839 Signed-off-by: Andrew Thornton <art27@cantab.net>
zeripath
added a commit
to zeripath/gitea
that referenced
this issue
May 31, 2022
Backport go-gitea#19843 In order for web.Wrap to be able to detect if a response has been written we need to wrap any non-context.ResponseWriters as a such. Otherwise responses will be incorrectly detected as non-written to and handlers can double run. In the case of GZip this handler will change the response to a non-context.RW and this failure to correctly detect response writing causes fallthrough and a NPE. Fix go-gitea#19839 Signed-off-by: Andrew Thornton <art27@cantab.net>
techknowlogick
added a commit
that referenced
this issue
Jun 3, 2022
* Ensure responses are context.ResponseWriters (#19843) Backport #19843 In order for web.Wrap to be able to detect if a response has been written we need to wrap any non-context.ResponseWriters as a such. Otherwise responses will be incorrectly detected as non-written to and handlers can double run. In the case of GZip this handler will change the response to a non-context.RW and this failure to correctly detect response writing causes fallthrough and a NPE. Fix #19839 Signed-off-by: Andrew Thornton <art27@cantab.net> * fix test Signed-off-by: Andrew Thornton <art27@cantab.net> Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com> Co-authored-by: techknowlogick <techknowlogick@gitea.io>
AbdulrhmnGhanem
pushed a commit
to kitspace/gitea
that referenced
this issue
Aug 24, 2022
In order for web.Wrap to be able to detect if a response has been written we need to wrap any non-context.ResponseWriters as a such. Otherwise responses will be incorrectly detected as non-written to and handlers can double run. In the case of GZip this handler will change the response to a non-context.RW and this failure to correctly detect response writing causes fallthrough and a NPE. Fix go-gitea#19839 Signed-off-by: Andrew Thornton <art27@cantab.net>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Steps:
server.ENABLE_GZIP=truehttp://localhost:3000/api/swaggerThe text was updated successfully, but these errors were encountered: