Set Content-Length of panic responses and flush them #168
Conversation
…ild." This reverts commit d469b8e.
When a service panics, we write an error into the http.ResponseWriter and re-raise the panic. If nothing up the http.Handler stack intercepts that panic, then the write might not get flushed in time, resulting in a confusing "EOF" error message for the client. We need to flush the error explicitly. But if only do that, then the Go HTTP library will switch into chunked transfer encoding mode. If that happens, the client doesn't know how many bytes to wait for in the error response, and the panicking server will close the HTTP response uncleanly, without terminating the chunked transfer response. As a result, if we just flush, the client _still_ just gets a confusing "EOF" message, since it's left unsure of whether the chunked transfer is done. The right thing is to explicitly set the content length of the response, which prevents the Go HTTP library from switching to chunked-transfer, and which informs the client of exactly how many bytes to expect. Only then can we flush.
protoc-gen-twirp/generator.go
Outdated
| t.P(` // Set Content-Length of response to avoid switching to chunked transfer`) | ||
| t.P(` // encoding when we flush, and so the client knows that it has received the`) | ||
| t.P(` // complete response when we flush.`) | ||
| t.P(` length := len(marshalErrorToJSON(twerr))`) |
There was a problem hiding this comment.
We end up calling marshalErrorToJSON twice in this code path - once here, and once in writeError. I could refactor to avoid that, but wanted the first revision to be clear, so I left in the double marshal.
There was a problem hiding this comment.
I think marshaling twice should be fine. Most error responses are small, most panics are unfrequent. If we ever find performance issues with this we can always refactor later.
There was a problem hiding this comment.
Why don't we send Content-Length on every error response? This could all move inside writeError, which currently calls resp.WriteHeader right before calling marshalErrorToJSON.
Why don't we send Content-Length on successful responses too? In those cases we definitely have the bytes ready before we write the header (so we can report any marshal errors).
There was a problem hiding this comment.
That's a good point, @rhysh. I like that. I'll revise to always set Content-Lengths.
| h := PanickyHatmaker("bang!") | ||
| s := httptest.NewUnstartedServer(NewHaberdasherServer(h, nil)) | ||
| defer s.Close() | ||
| s.Config.ErrorLog = testLogger(t) |
There was a problem hiding this comment.
Is not clear what is being tested or setup here. Maybe this line needs a comment explaining what to expect?
There was a problem hiding this comment.
You could change the input to func(args ...interface{}) and pass in t.Log to be more explicit about how t can be used.
| t.P(` f, ok := resp.(http.Flusher)`) | ||
| t.P(` if ok {`) | ||
| t.P(` f.Flush()`) | ||
| t.P(` }`) |
There was a problem hiding this comment.
style nitpick: mixed spaces and tabs
| if twerr.Code() != twirp.Internal { | ||
| t.Errorf("twirp ErrorCode expected to be %q, but found %q", twirp.Internal, twerr.Code()) | ||
| } | ||
| if twerr.Msg() != "Internal service panic" { |
There was a problem hiding this comment.
Instead of repeating the desired value on the next line, consider if have, want := twerr.Msg(), "Internal service panic"; have != want {
| h := PanickyHatmaker("bang!") | ||
| s := httptest.NewUnstartedServer(NewHaberdasherServer(h, nil)) | ||
| defer s.Close() | ||
| s.Config.ErrorLog = testLogger(t) |
There was a problem hiding this comment.
You could change the input to func(args ...interface{}) and pass in t.Log to be more explicit about how t can be used.
protoc-gen-twirp/generator.go
Outdated
| t.P(` // Set Content-Length of response to avoid switching to chunked transfer`) | ||
| t.P(` // encoding when we flush, and so the client knows that it has received the`) | ||
| t.P(` // complete response when we flush.`) | ||
| t.P(` length := len(marshalErrorToJSON(twerr))`) |
There was a problem hiding this comment.
Why don't we send Content-Length on every error response? This could all move inside writeError, which currently calls resp.WriteHeader right before calling marshalErrorToJSON.
Why don't we send Content-Length on successful responses too? In those cases we definitely have the bytes ready before we write the header (so we can report any marshal errors).
|
This PR now adds Content-Length to all responses. I think this is just better. Thanks for the prod, @rhysh. |
Issue #, if available:
#162
Description of changes:
This changes panic handling in generated Twirp servers. They now calculate the length of the response, set it in the Content-Length header of the response, and explicitly flush.
When a service panics, we write an error into the
http.ResponseWriterand re-raise the panic. If nothing up thehttp.Handlerstack intercepts that panic, then the write might not get flushed in time, resulting in a confusing "EOF" error message for the client.We need to flush the error explicitly. But if we only do that, then the Go HTTP library will switch into chunked transfer encoding mode. If that happens, the client doesn't know how many bytes to wait for in the error response, and the panicking server will close the HTTP response uncleanly, without terminating the chunked transfer response. As a result, if we just flush, the client still just gets a
confusing "EOF" message, since it's left unsure of whether the chunked transfer is done.
The right thing is to explicitly set the content length of the response, which prevents the Go HTTP library from switching to chunked-transfer, and which informs the client of exactly how many bytes to expect. Only then can we flush.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.