-
Notifications
You must be signed in to change notification settings - Fork 18.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Avoid panic on write after close in http #17634
Conversation
@@ -63,7 +63,9 @@ func (s *router) getContainersStats(ctx context.Context, w http.ResponseWriter, | |||
w.Header().Set("Content-Type", "application/json") | |||
out = w | |||
} else { | |||
out = ioutils.NewWriteFlusher(w) | |||
wf := ioutils.NewWriteFlusher(w) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder what does this mean :)
Why not
out = ioutils.NewWriteFlusher(w)
defer out.Close()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, got it.
@stevvooe tests are failing |
8281b87
to
1f7344d
Compare
n, err = wf.w.Write(b) | ||
wf.flushed = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason for removing this here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(*WriteFlusher).Flush()
sets it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is calling (http.Flusher).Flush()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry, this is terrible code. Fixing now.
13b4886
to
ca6bd7e
Compare
By adding a (*WriteFlusher).Close, we limit the Write calls to possibly deallocated http response buffers to the lifetime of an http request. Typically, this is seen as a very confusing panic, the cause is usually a situation where an http.ResponseWriter is held after request completion. We avoid the panic by disallowing further writes to the response writer after the request is completed. Signed-off-by: Stephen J Day <stephen.day@docker.com>
ca6bd7e
to
ec2289b
Compare
LGTM |
1 similar comment
LGTM |
Avoid panic on write after close in http
By adding a (*WriteFlusher).Close, we limit the Write calls to possibly
deallocated http response buffers to the lifetime of an http request.
Typically, this is seen as a very confusing panic, the cause is usually a
situation where an http.ResponseWriter is held after request completion. We
avoid the panic by disallowing further writes to the response writer after the
request is completed.
Closes #17625.
Signed-off-by: Stephen J Day stephen.day@docker.com