-
Notifications
You must be signed in to change notification settings - Fork 998
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
Expose Error Status To Caller #1682
Conversation
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.
LGTM thanks!
One unfortunate outcome of this change seems to be that once an error occurs, the Stream will yield that error forever instead of terminating the stream on error as was done before. |
@bmwill that sounds bad -- want to submit a PR to fix it? |
@djc yeah happy to take a look at addressing the issue, assuming the behavior is a regression that the project would like fixed and not an change in behavior that it would like to keep. |
I think that's a good assumption! |
PR hyperium#1682 introduced a change to return the error Status to the user of Streaming but resulted in a change in behavior such that if an error occurs the Stream will never end and instead always yeild the error. This patch adjusts the semantics to only return the error one time and then terminates the stream by returning Poll::Ready(None) on subseqent calls.
PR hyperium#1682 introduced a change to return the error Status to the user of Streaming but resulted in a change in behavior such that if an error occurs the Stream will never end and instead always yeild the error. This patch adjusts the semantics to only return the error one time and then terminates the stream by returning Poll::Ready(None) on subseqent calls.
) PR #1682 introduced a change to return the error Status to the user of Streaming but resulted in a change in behavior such that if an error occurs the Stream will never end and instead always yeild the error. This patch adjusts the semantics to only return the error one time and then terminates the stream by returning Poll::Ready(None) on subseqent calls.
Motivation
We were seeing a high rate
Missing request message
error on server side, but we do not know what exactly was going on.I think the error message may be hidden away in
poll_data
.I am not sure why tonic choose to drop the error message instead of returning it to the user.
Can people familiar with the code explain why it was done in the current way?
Solution
return the error to users, so the error is more meaningful