Skip to content

Commit

Permalink
Fix multiple header writes for connect unary on error (#726)
Browse files Browse the repository at this point in the history
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 calling `WriteHeader`. If the body has already been
written this triggers the following log via `http.Server`:
```
http: superfluous response.WriteHeader call from connectrpc.com/connect.(*connectUnaryHandlerConn).Close (protocol_connect.go:743)
```
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 `wroteBody` check to a `wroteHeader` check 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.
  • Loading branch information
emcfarlane authored Apr 17, 2024
1 parent 1abde82 commit fd9d60a
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 10 deletions.
7 changes: 4 additions & 3 deletions connect.go
Original file line number Diff line number Diff line change
Expand Up @@ -431,9 +431,10 @@ func receiveUnaryMessage[T any](conn receiveConn, initializer maybeInitializer,
if err := initializer.maybe(conn.Spec(), &msg2); err != nil {
return nil, err
}
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) {
if err == nil {
err = NewError(CodeUnimplemented, fmt.Errorf("unary %s has multiple messages", what))
}
return nil, err
}
return &msg, nil
Expand Down
14 changes: 7 additions & 7 deletions protocol_connect.go
Original file line number Diff line number Diff line change
Expand Up @@ -686,7 +686,6 @@ type connectUnaryHandlerConn struct {
marshaler connectUnaryMarshaler
unmarshaler connectUnaryUnmarshaler
responseTrailer http.Header
wroteBody bool
}

func (hc *connectUnaryHandlerConn) Spec() Spec {
Expand All @@ -709,8 +708,7 @@ func (hc *connectUnaryHandlerConn) RequestHeader() http.Header {
}

func (hc *connectUnaryHandlerConn) Send(msg any) error {
hc.wroteBody = true
hc.writeResponseHeader(nil /* error */)
hc.mergeResponseHeader(nil /* error */)
if err := hc.marshaler.Marshal(msg); err != nil {
return err
}
Expand All @@ -726,16 +724,16 @@ func (hc *connectUnaryHandlerConn) ResponseTrailer() http.Header {
}

func (hc *connectUnaryHandlerConn) Close(err error) error {
if !hc.wroteBody {
hc.writeResponseHeader(err)
if !hc.marshaler.wroteHeader {
hc.mergeResponseHeader(err)
// 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 {
return hc.request.Body.Close()
}
// In unary Connect, errors always use application/json.
Expand All @@ -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) {
header := hc.responseWriter.Header()
if hc.request.Method == http.MethodGet {
// The response content varies depending on the compression that the client
Expand Down Expand Up @@ -923,6 +921,7 @@ type connectUnaryMarshaler struct {
bufferPool *bufferPool
header http.Header
sendMaxBytes int
wroteHeader bool
}

func (m *connectUnaryMarshaler) Marshal(message any) *Error {
Expand Down Expand Up @@ -961,6 +960,7 @@ func (m *connectUnaryMarshaler) Marshal(message any) *Error {
}

func (m *connectUnaryMarshaler) write(data []byte) *Error {
m.wroteHeader = true
payload := bytes.NewReader(data)
if _, err := m.sender.Send(payload); err != nil {
err = wrapIfContextError(err)
Expand Down

0 comments on commit fd9d60a

Please sign in to comment.