Fix multiple header writes for connect unary on error #726
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
emcfarlane
commented
Apr 17, 2024
| if err := conn.Receive(&msg2); err == nil { | ||
| return nil, NewError(CodeUnimplemented, fmt.Errorf("unary %s has multiple messages", what)) | ||
| } else if err != nil && !errors.Is(err, io.EOF) { | ||
| if err := conn.Receive(&msg2); !errors.Is(err, io.EOF) { |
There was a problem hiding this comment.
Unrelated to the change but the error check err != nil was superfluous, so simplified to the happy path first.
emcfarlane
commented
Apr 17, 2024
| @@ -757,7 +755,7 @@ func (hc *connectUnaryHandlerConn) getHTTPMethod() string { | |||
| return hc.request.Method | |||
| } | |||
|
|
|||
| func (hc *connectUnaryHandlerConn) writeResponseHeader(err error) { | |||
| func (hc *connectUnaryHandlerConn) mergeResponseHeader(err error) { | |||
There was a problem hiding this comment.
Renamed to mergeResponseHeaders as this doesn't trigger a write.
emcfarlane
commented
Apr 17, 2024
| // If the handler received a GET request and the resource hasn't changed, | ||
| // return a 304. | ||
| if len(hc.peer.Query) > 0 && IsNotModifiedError(err) { | ||
| hc.responseWriter.WriteHeader(http.StatusNotModified) | ||
| return hc.request.Body.Close() | ||
| } | ||
| } | ||
| if err == nil { | ||
| if err == nil || hc.marshaler.wroteHeader { |
There was a problem hiding this comment.
We could error here to report to the Close that the error wasn't encoded. I've left for now, as a best effort solution.
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
On error a connect unary handler will call
Close(err)to send the final error. For non-nil errors this will always attempt to encode the header status by callingWriteHeader. If the body has already been written this triggers the following log viahttp.Server:The common case for this superfluous log is when a message send is interrupted, due to a context cancel or other write error, and the error is then attempting to re-encode the headers and body.
This PR now moves the
wroteBodycheck to awroteHeadercheck on the unmarshaller. When any payload is sent, including a nil field for header only, we now stop attempting to encode any following errors, as it would be superfluous.