Skip to content

fix: ensure exception is available when BackgroundConsumer open stream fails #357

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

Merged
parthea merged 12 commits into from
Oct 17, 2023
Merged

fix: ensure exception is available when BackgroundConsumer open stream fails #357

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
1a64fff
fix: ensure exception is available when BackgroundConsumer open strea…
parthea Mar 14, 2022
97c2363
chore: fix coverage
parthea Mar 15, 2022
2b24714
Merge branch 'main' into fix-background-consumer-start-eats-exception
parthea Jun 14, 2022
947b657
Merge branch 'main' into fix-background-consumer-start-eats-exception
parthea Dec 28, 2022
ad1f29f
Merge branch 'main' into fix-background-consumer-start-eats-exception
rosiezou Feb 3, 2023
7ec7832
Merge branch 'main' into fix-background-consumer-start-eats-exception
parthea Sep 1, 2023
c7f63bd
Address review comments
parthea Oct 13, 2023
3304ef1
revert
parthea Oct 13, 2023
e120a0c
address review feedback
parthea Oct 13, 2023
753a591
raise grpc.RpcError instead of GoogleAPICallError
parthea Oct 13, 2023
3c1daa9
🦉 Updates from OwlBot post-processor
gcf-owl-bot[bot] Oct 13, 2023
9423063
Merge branch 'main' into fix-background-consumer-start-eats-exception
parthea Oct 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion google/api_core/bidi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,11 @@ def open(self):
request_generator = _RequestQueueGenerator(
self._request_queue, initial_request=self._initial_request
)
call = self._start_rpc(iter(request_generator), metadata=self._rpc_metadata)
try:
Copy link
Contributor

@vchudnov-g vchudnov-g Sep 28, 2023

Choose a reason for hiding this comment

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

In examining this file, it seems to me that _RequestQueueGenerator._is_active (lines 91 et seq) should be defd as return self.call is not None and self.call.is_active(). The way it's currently written it will return True when self.call == None

Copy link
Collaborator Author

@parthea parthea Oct 13, 2023

Choose a reason for hiding this comment

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

Fixed in e120a0c

call = self._start_rpc(iter(request_generator), metadata=self._rpc_metadata)
except exceptions.GoogleAPICallError as exc:
self._on_call_done(exc)
Copy link
Contributor

@atulep atulep Apr 26, 2022

Choose a reason for hiding this comment

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

on_call_done expects a future (based on the name of its argument). How does it work with an exception here?

Copy link
Collaborator Author

@parthea parthea Jun 14, 2022

Choose a reason for hiding this comment

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

The exception will be raised in the call back add_done_callback and must be handled by the user. This is based on the comment here:

python-api-core/google/api_core/bidi.py

Lines 608 to 610 in 8f73d2e

Note that error handling *must* be done by using the provided
``bidi_rpc``'s ``add_done_callback``. This helper will automatically exit
whenever the RPC itself exits and will not provide any error details.

Copy link
Contributor

@vchudnov-g vchudnov-g Sep 28, 2023

Choose a reason for hiding this comment

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

add_done_callback specifies callback (Callable[[grpc.Future], None]), but it's not clear to me that GoogleAPICallError is a grpc.Future (see this)

Copy link
Collaborator Author

@parthea parthea Oct 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The change that I proposed is exactly the same behaviour as the _on_call_done method of ResumableBidiRpc

python-api-core/google/api_core/bidi.py

Lines 441 to 443 in 6251eab

# Unlike the base class, we only execute the callbacks on a terminal
# error, not for errors that we can recover from. Note that grpc's
# "future" here is also a grpc.RpcError.

From grpc/grpc#10885 (comment), grpc.RpcError is also grpc.Call.

I added this note in c7f63bd

Copy link
Collaborator Author

@parthea parthea Oct 13, 2023

Choose a reason for hiding this comment

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

I reverted c7f63bd because GoogleAPICallError is not grpc.Call or grpc.RpcError. The original grpc.RpcError is available on the response property of GoogleAPICallError

python-api-core/google/api_core/grpc_helpers.py

Lines 174 to 182 in 6251eab

def wrap_errors(callable_):
"""Wrap a gRPC callable and map :class:`grpc.RpcErrors` to friendly error
classes.
Errors raised by the gRPC callable are mapped to the appropriate
:class:`google.api_core.exceptions.GoogleAPICallError` subclasses.
The original `grpc.RpcError` (which is usually also a `grpc.Call`) is
available from the ``response`` property on the mapped exception. This
is useful for extracting metadata from the original error.

The behaviour proposed in this PR is the same as ResumableBidiRpc and we typically raise GoogleAPICallError instead of grpc.RpcError

Copy link
Collaborator Author

@parthea parthea Oct 13, 2023

Choose a reason for hiding this comment

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

I changed the code in 753a591 to raise grpc.RpcError instead of GoogleAPICallError since grpc.RpcError is also grpc.Call and it is a better fit

raise

request_generator.call = call

Expand Down
13 changes: 13 additions & 0 deletions tests/unit/test_bidi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -804,6 +804,19 @@ def test_wake_on_error(self):
while consumer.is_active:
pass

def test_rpc_callback_fires_when_consumer_start_fails(self):
expected_exception = exceptions.InvalidArgument("test")
callback = mock.Mock(spec=["__call__"])

rpc, _ = make_rpc()
bidi_rpc = bidi.BidiRpc(rpc)
bidi_rpc.add_done_callback(callback)
bidi_rpc._start_rpc.side_effect = expected_exception

consumer = bidi.BackgroundConsumer(bidi_rpc, on_response=None)
consumer.start()
assert callback.call_args.args[0] == expected_exception

def test_consumer_expected_error(self, caplog):
caplog.set_level(logging.DEBUG)

Expand Down