-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
return err early if EOF to prevent logging in normal conditions #191
Conversation
This matches the client streaming case more closely
@@ -248,6 +248,9 @@ var ( | |||
handleSend := func() error { | |||
var protoReq {{.Method.RequestType.GoType .Method.Service.File.GoPkg.Path}} | |||
err = dec.Decode(&protoReq) | |||
if err == io.EOF { |
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.
Is it better to handle io.EOF
at L264 if err := handleSend(); err != nil
and return nil
instead of io.EOF
?
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.
@yugui I was aiming for code parity with the other handling code
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.
Code parity is fine.
But I am wondering if it falls into https://github.com/tmc/grpc-gateway/blob/b069c864828f47dac69037b625ae20953d4cd430/examples/examplepb/stream.pb.gw.go#L245 even though it is valid to send a zero-length sequence.
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.
@yugui thoughts on doing an io.EOF check before calling HTTPError there?
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.
@tmc There's no guarantee that io.EOF
is/will be coming only from dec.Decode
.
The best place to deal with an error is the place the error happens.
cc #195 |
@yugui Could you give us ETA on this? etcd team assumed that this would be merged by next Wednesday, and postponed the release that was scheduled today. Thanks! |
@yugui I imagine we'll be changing this code a bit with the discussion around the special treatment of the first message for bidi endpoints -- my take is get this in and we can discuss zero-length send streams separately. |
@tmc Sounds reasonable. I'll merge this PR. |
return err early if EOF to prevent logging in normal conditions
This matches the client streaming case more closely.