Skip to content
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

Recover from panic in body write #687

Merged

Conversation

@Bobochka
Copy link
Contributor

Bobochka commented Nov 3, 2019

Possibly a successor of #646.
Went as per @erikdubbelboer suggestion (if I understood it correctly). If the general idea is acceptable, will add tests.
Thanks.

Bobochka added 2 commits Nov 3, 2019
Update from upstream
Copy link
Collaborator

erikdubbelboer left a comment

This looks good yes, please add a test.

server.go Outdated
@@ -2219,6 +2219,11 @@ func writeResponse(ctx *RequestCtx, w *bufio.Writer) error {
panic("BUG: cannot write timed out response")
}
err := ctx.Response.Write(w)
if err != nil {
if panicErr, ok := err.(*ErrBodyStreamWritePanic); ok {
ctx.logger.Printf("panic while writing response: %s", panicErr.Error())

This comment has been minimized.

Copy link
@erikdubbelboer

erikdubbelboer Nov 3, 2019

Collaborator

I don't think its necessary to log this here. The error will go all the way up to workerpool.go#L230 where it will be logged anyways.

@Bobochka

This comment has been minimized.

Copy link
Contributor Author

Bobochka commented Nov 3, 2019

@erikdubbelboer done.

Unfortunately this doesn't cover SetBodyStreamWriter as writer invocation happens in another goroutine.

@erikdubbelboer

This comment has been minimized.

Copy link
Collaborator

erikdubbelboer commented Nov 6, 2019

I think we can catch the SetBodyStreamWriter as well if we do a similar trick as io.PipeWriter.CloseWithError(). But this will be a bit complicated as fasthttputil.NewPipeConns() deals with net.Conn and not some interface we can just add CloseWithError() to.

So lets save that for another pull request.

@erikdubbelboer erikdubbelboer merged commit 70223a1 into valyala:master Nov 6, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.