Skip to content
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

handling and returning valid error code for context.Canceled and cont… #294

Merged
merged 27 commits into from
Apr 9, 2021

Conversation

chetanvalagit
Copy link
Contributor

…ext.DeadlineExceeded error

*Issue #, if available: #293

Description of changes:
Proper error code return in case of context.Canceled (Error code will be returned twirp.Canceled) and context.DeadlineExceeded (Error code will be returned twirp. DeadlineExceeded), instead of 5xx 'Unexpected EOF'

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@chetanvalagit chetanvalagit added the bug Something isn't working label Mar 8, 2021
@chetanvalagit chetanvalagit marked this pull request as draft March 8, 2021 20:07
@chetanvalagit chetanvalagit marked this pull request as ready for review March 11, 2021 06:24
Copy link
Contributor

@marioizquierdo marioizquierdo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Let's release this as v7.2.0

Copy link
Contributor

@rhysh rhysh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The behavior change LGTM, but the tests are too far removed from how Twirp gets used in production. The tests should verify that we get the right behavior higher up in the stack, and demonstrate to readers (including our future selves) what that high-level behavior is.

Copy link
Contributor

@rhysh rhysh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for restructuring and expanding the test code. I don't think that any of my remaining comments are deal-breakers. They'd be good to change, but don't need another round of review.

Copy link
Contributor

@marioizquierdo marioizquierdo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@chetanvalagit chetanvalagit merged commit ab1c37c into master Apr 9, 2021
@chetanvalagit chetanvalagit deleted the cv_issue_293 branch April 9, 2021 05:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: Check request context before returning internal error on server handler
3 participants