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
server: return better status for context err when writing header #5292
Changes from 2 commits
1715f7e
6f33edb
e38bde5
732ffc9
f739f9e
32d8958
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,7 +21,6 @@ package transport | |
import ( | ||
"bytes" | ||
"context" | ||
"errors" | ||
"fmt" | ||
"io" | ||
"math" | ||
|
@@ -53,10 +52,10 @@ import ( | |
var ( | ||
// ErrIllegalHeaderWrite indicates that setting header is illegal because of | ||
// the stream's state. | ||
ErrIllegalHeaderWrite = errors.New("transport: the stream is done or WriteHeader was already called") | ||
ErrIllegalHeaderWrite = status.Error(codes.Internal, "transport: the stream is done or WriteHeader was already called") | ||
// ErrHeaderListSizeLimitViolation indicates that the header list size is larger | ||
// than the limit set by peer. | ||
ErrHeaderListSizeLimitViolation = errors.New("transport: trying to send header list size larger than the limit set by peer") | ||
ErrHeaderListSizeLimitViolation = status.Error(codes.Internal, "transport: trying to send header list size larger than the limit set by peer") | ||
) | ||
|
||
// serverConnectionCounter counts the number of connections a server has seen | ||
|
@@ -933,9 +932,14 @@ func (t *http2Server) checkForHeaderListSize(it interface{}) bool { | |
|
||
// WriteHeader sends the header metadata md back to the client. | ||
func (t *http2Server) WriteHeader(s *Stream, md metadata.MD) error { | ||
if s.updateHeaderSent() || s.getState() == streamDone { | ||
if s.getState() == streamDone { | ||
return ContextErr(s.ctx.Err()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's a small window where the state can be We could paper over this race by calling Looking closer at func (t *http2Server) streamContextErr(s *Stream) error {
select {
case <-t.done:
return ErrConnClosing
default:
}
return ContextErr(s.ctx.Err())
} But I can do that as a follow-on later otherwise. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup, tests seem to all pass with this! I changed it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is done by |
||
} | ||
|
||
if s.updateHeaderSent() { | ||
dfawley marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return ErrIllegalHeaderWrite | ||
} | ||
|
||
s.hdrMu.Lock() | ||
if md.Len() > 0 { | ||
if s.header.Len() > 0 { | ||
|
@@ -1062,11 +1066,7 @@ func (t *http2Server) WriteStatus(s *Stream, st *status.Status) error { | |
func (t *http2Server) Write(s *Stream, hdr []byte, data []byte, opts *Options) error { | ||
if !s.isHeaderSent() { // Headers haven't been written yet. | ||
if err := t.WriteHeader(s, nil); err != nil { | ||
if _, ok := err.(ConnectionError); ok { | ||
return err | ||
} | ||
// TODO(mmukhi, dfawley): Make sure this is the right code to return. | ||
return status.Errorf(codes.Internal, "transport: %v", err) | ||
return err | ||
dfawley marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} else { | ||
// Writing headers checks for this condition. | ||
|
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.
Please update this text since we're more precise about sending this error now. E.g.
"stream.SendHeader called multiple times"
. (It'sSendHeader
at the user API level.)