[go: nahoru, domu]
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.

Sign up for GitHub

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

Jump to bottom

Firestore: Add 'should_terminate' predicate for clean BiDi shutdown. #8650

Merged
merged 7 commits into from
Jul 17, 2019
Merged

Firestore: Add 'should_terminate' predicate for clean BiDi shutdown. #8650

merged 7 commits into from
Jul 17, 2019

Conversation

tseaver
Copy link
Contributor
@tseaver tseaver commented Jul 11, 2019

Stream exceptions which pass this predicate will cause clean shutdown of the BiDi thread.

Closes #7826.

Note to reviewers: I recommend looking at this commit-by-commit.

@tseaver tseaver added api: core api: firestore Issues related to the Firestore API. labels Jul 11, 2019
@tseaver tseaver requested a review from frankyn as a code owner July 11, 2019 20:23
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 11, 2019
@tseaver
Copy link
Contributor Author
tseaver commented Jul 11, 2019

I can also add should_terminate support for Pubsub, if desired (and someone can confirm the set of gRPC status codes which should cause clean shutdown).

@crwilcox
Copy link
Contributor
crwilcox commented Jul 11, 2019 •
edited
Loading

Is there a reason should_recover couldn't be used for this? Semantically, should not recover and should terminate seem the same? Do we need another argument.

Also, note, immediately following this merge, we should ship api_core and rev min versions for firestore and pubsub

@tseaver
Copy link
Contributor Author
tseaver commented Jul 12, 2019

@crwilcox See #7826. The goal is to avoid logging an error when the stream is shut down due to "expected" events., typically due to a grpc.StatusCode.CANCELLED error response

  • The existing should_recover returns True if the error is "recoverable", i.e., the stream should save any pending send and reopen itself.
  • The new should_terminate returns True if the error should cause the stream to shut down cleanly, without reopening.

plamut
plamut reviewed Jul 15, 2019
View reviewed changes
Copy link
Contributor
@plamut plamut left a comment

Choose a reason for hiding this comment

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

The code changes look good to me.

I was able to reproduce the issue as described, but after re-installing api_core and the firestore changes from this PR, I observed a different error:

(venv-3.6) peter@black-box:~/workspace/google-cloud-python/firestore (pr_temp)$ python reproduce-7826.py 
on_snapshot()
Thread-ConsumeBidirectionalStream caught unexpected exception 'NoneType' object has no attribute 'target_change' and will exit.
Traceback (most recent call last):
  File "/home/peter/workspace/google-cloud-python/firestore/venv-3.6/lib/python3.6/site-packages/google/api_core/bidi.py", line 653, in _thread_main
    self._on_response(response)
  File "/home/peter/workspace/google-cloud-python/firestore/venv-3.6/lib/python3.6/site-packages/google/cloud/firestore_v1/watch.py", line 429, in on_snapshot
    target_change = proto.target_change
AttributeError: 'NoneType' object has no attribute 'target_change'

It might be caused by my local/project setup, but it might as well be unrelated to the error reported in the ticket (which seems to be fixed by this PR).

@tseaver
Copy link
Contributor Author
tseaver commented Jul 15, 2019

@plamut Maybe the patch from this PR allows a final None to be passed through on a cancelled stream, where previoiusly it would have errored out with the traceback from #7826?

@tseaver
Copy link
Contributor Author
tseaver commented Jul 15, 2019

In any case, if it is possible that on_snapshot can be called with None as the response, we need to handle that (by an immediate return, I think).

This was referenced Jul 16, 2019
Closed
Merged
plamut
plamut approved these changes Jul 17, 2019
View reviewed changes
Copy link
Contributor
@plamut plamut left a comment

Choose a reason for hiding this comment

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

Applying the changes from #8687, the AttributeError is now gone, and the fix for the original issue still works.

@tseaver tseaver merged commit 94043b1 into googleapis:master Jul 17, 2019
@tseaver tseaver deleted the 7826-api_core-firestore-add-should_terminate-to-bidi branch July 17, 2019 17:03
tseaver added a commit that referenced this pull request Jul 24, 2019
@tseaver
Treat 'None' as EOF in 'Watch.on_snapshot'. (#8687)
1dc92c7 
See:
#8650 (review)
crwilcox pushed a commit to googleapis/python-firestore that referenced this pull request Jan 31, 2020
@tseaver
Treat 'None' as EOF in 'Watch.on_snapshot'. (#8687)
a6f220e 
See:
googleapis/google-cloud-python#8650 (review)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@plamut plamut plamut approved these changes

@crwilcox crwilcox Awaiting requested review from crwilcox

@busunkim96 busunkim96 Awaiting requested review from busunkim96

@frankyn frankyn Awaiting requested review from frankyn

Assignees
No one assigned
Labels
api: core api: firestore Issues related to the Firestore API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Firestore: watch unsubscribe generates unexpected exception
4 participants
@tseaver @crwilcox @plamut @googlebot