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

fix(pubsub): do not propagate context deadline exceeded error #3055

Merged
merged 3 commits into from
Oct 21, 2020

Conversation

hongalex
Copy link
Member

In certain scenarios (usually on network disconnect but to be fully determined), an unknown context deadline exceeded error from ModifyAckDeadline can shutdown the iterator and cause it to fail. This is usually captured by the case prior to default in the switch statement, but sometimes the error is not captured properly. This PR patches the issue so users are not negatively affected.

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Oct 20, 2020
@product-auto-label product-auto-label bot added the api: pubsub Issues related to the Pub/Sub API. label Oct 20, 2020
pubsub/iterator.go Show resolved Hide resolved
pubsub/iterator.go Show resolved Hide resolved
pubsub/iterator.go Show resolved Hide resolved
pubsub/iterator.go Show resolved Hide resolved
@hongalex
Copy link
Member Author

hongalex commented Oct 20, 2020

Upon further investigation, the error seems to be originating from the gax.Sleep call. The context is cancelled, and a fallthrough happens. However, since the err is not checked directly (rather the previously stored error code which is a Unavailable), the context deadline exceeded error is bubbled up to the client library user. Will be updating this PR to address.

edit: This is wrong. The gax.Sleep deadline is properly falling through to the next call, which means this can't be the source of context deadline exceeded errors. However, some context deadline exceeded errors are not captured by the switch case (https://play.golang.org/p/rgJjhB-BLl-).

@noahdietz
Copy link
Contributor

the error seems to be originating from the gax.Sleep call

Interesting, is there anything that needs to change in gax? Or is there a bug in gax?

Copy link
Contributor

@noahdietz noahdietz left a comment

Choose a reason for hiding this comment

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

LGTM but I'll let Cody Approve the PR.

Copy link
Member

@codyoss codyoss left a comment

Choose a reason for hiding this comment

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

LGTM

@hongalex hongalex merged commit ddcca43 into googleapis:master Oct 21, 2020
@hongalex hongalex deleted the cps-modack branch October 21, 2020 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: pubsub Issues related to the Pub/Sub API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants