-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Firestore: Add 'should_terminate' predicate for clean BiDi shutdown. #8650
Firestore: Add 'should_terminate' predicate for clean BiDi shutdown. #8650
Conversation
Sharpen assertions for other '_on_call_done' testcases.
I can also add |
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 |
@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
|
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.
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).
In any case, if it is possible that |
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.
Applying the changes from #8687, the AttributeError
is now gone, and the fix for the original issue still works.
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.