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

parthea · 2022-03-14T20:37:45Z

Fixes #268🦕

This PR updates the bidi.py code to reflect what is stated in the comments.

The comment for add_done_callback states This occurs when the RPC errors or is successfully terminated. however the done callback was not being called when an error occurs.

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

Lines 255 to 264 in 2477ab9

    
               def add_done_callback(self, callback): 
        
                   """Adds a callback that will be called when the RPC terminates. 
        
                   This occurs when the RPC errors or is successfully terminated. 
        
                   Args: 
        
                       callback (Callable[[grpc.Future], None]): The callback to execute. 
        
                           It will be provided with the same gRPC future as the underlying 
        
                           stream which will also be a :class:`grpc.Call`. 
        
                   """

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

Lines 267 to 269 in 2477ab9

    
           def _on_call_done(self, future): 
        
               for callback in self._callbacks: 
        
                   callback(future)

…m fails

tswast

This looks great! Let's block this until we figure out google-cloud-bigquery-storage though. googleapis/python-bigquery-storage#414

google/api_core/bidi.py

atulep · 2022-04-26T20:05:05Z

google/api_core/bidi.py

+        try:
+            call = self._start_rpc(iter(request_generator), metadata=self._rpc_metadata)
+        except exceptions.GoogleAPICallError as exc:
+            self._on_call_done(exc)


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

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.

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

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

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

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

Dismiss stale review

parthea · 2022-06-30T02:22:56Z

@loferris, please could you review/approve?

tswast · 2023-01-24T22:02:59Z

@rosiezou You might be interested in this PR. I forget the full context, but I know it's something I wanted for better debugging of the Python BQ Storage Write API.

brianhlee · 2023-09-19T17:31:54Z

@parthea any idea when this will be merged?

parthea · 2023-09-19T19:05:23Z

@vchudnov-g Please could you take a look?

vchudnov-g · 2023-09-28T18:12:16Z

google/api_core/bidi.py

@@ -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:


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

Fixed in e120a0c

vchudnov-g · 2023-09-28T18:30:38Z

google/api_core/bidi.py

+        try:
+            call = self._start_rpc(iter(request_generator), metadata=self._rpc_metadata)
+        except exceptions.GoogleAPICallError as exc:
+            self._on_call_done(exc)


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

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

parthea · 2023-10-13T18:48:38Z

@vchudnov-g Please could you take another look?

This reverts commit e120a0c.

…eam caught unexpected exception and will exit` (#562) * chore: partial revert of PR #357 This reverts commit e120a0c. * add comment

parthea requested review from a team as code owners March 14, 2022 20:37

parthea force-pushed the fix-background-consumer-start-eats-exception branch from 7f05e51 to 767b215 Compare March 14, 2022 20:38

fix: ensure exception is available when BackgroundConsumer open strea…

1a64fff

…m fails

parthea force-pushed the fix-background-consumer-start-eats-exception branch from 767b215 to 1a64fff Compare March 14, 2022 20:40

parthea mentioned this pull request Mar 14, 2022

fix: handle AttributeError in bigquery_storage writer googleapis/python-bigquery-storage#414

Merged

chore: fix coverage

97c2363

parthea force-pushed the fix-background-consumer-start-eats-exception branch from dddb4d3 to 97c2363 Compare March 15, 2022 13:18

tswast previously requested changes Mar 15, 2022

View reviewed changes

atulep reviewed Apr 26, 2022

View reviewed changes

google/api_core/bidi.py Show resolved Hide resolved

atulep reviewed Apr 26, 2022

View reviewed changes

Merge branch 'main' into fix-background-consumer-start-eats-exception

2b24714

product-auto-label bot added the size: s Pull request size is small. label Jun 14, 2022

parthea requested review from tswast and atulep June 24, 2022 15:31

parthea assigned loferris Jun 30, 2022

parthea requested a review from loferris June 30, 2022 02:22

parthea requested a review from steffnay June 30, 2022 02:24

parthea assigned steffnay Jun 30, 2022

Merge branch 'main' into fix-background-consumer-start-eats-exception

947b657

tswast requested a review from rosiezou January 24, 2023 22:02

rosiezou and others added 2 commits February 3, 2023 11:50

Merge branch 'main' into fix-background-consumer-start-eats-exception

ad1f29f

Merge branch 'main' into fix-background-consumer-start-eats-exception

7ec7832

parthea assigned vchudnov-g and unassigned steffnay and loferris Sep 1, 2023

vchudnov-g reviewed Sep 28, 2023

View reviewed changes

parthea assigned parthea and unassigned vchudnov-g Sep 29, 2023

parthea and others added 6 commits October 13, 2023 16:59

Address review comments

c7f63bd

revert

3304ef1

address review feedback

e120a0c

raise grpc.RpcError instead of GoogleAPICallError

753a591

🦉 Updates from OwlBot post-processor

3c1daa9

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

Merge branch 'main' into fix-background-consumer-start-eats-exception

9423063

parthea assigned vchudnov-g and unassigned parthea Oct 13, 2023

tswast removed request for tswast, rosiezou and atulep October 13, 2023 21:05

vchudnov-g approved these changes Oct 17, 2023

View reviewed changes

parthea merged commit 405272c into main Oct 17, 2023
24 checks passed

parthea deleted the fix-background-consumer-start-eats-exception branch October 17, 2023 19:54

release-please bot mentioned this pull request Oct 17, 2023

chore(main): release 2.13.0 #537

Merged

parthea added a commit that referenced this pull request Nov 30, 2023

chore: partial revert of PR #357

536c6d2

This reverts commit e120a0c.

parthea added a commit that referenced this pull request Dec 1, 2023

fix: fix regression in bidi causing `Thread-ConsumeBidirectionalStr…

40c8ae0

…eam caught unexpected exception and will exit` (#562) * chore: partial revert of PR #357 This reverts commit e120a0c. * add comment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

parthea commented Mar 14, 2022 •

edited

Loading

tswast left a comment

atulep Apr 26, 2022

parthea Jun 14, 2022

vchudnov-g Sep 28, 2023

parthea Oct 13, 2023 •

edited

Loading

parthea Oct 13, 2023

parthea Oct 13, 2023

parthea commented Jun 30, 2022

tswast commented Jan 24, 2023

brianhlee commented Sep 19, 2023

parthea commented Sep 19, 2023

vchudnov-g Sep 28, 2023

parthea Oct 13, 2023

vchudnov-g Sep 28, 2023

parthea commented Oct 13, 2023

	def add_done_callback(self, callback):
	"""Adds a callback that will be called when the RPC terminates.

	This occurs when the RPC errors or is successfully terminated.

	Args:
	callback (Callable[[grpc.Future], None]): The callback to execute.
	It will be provided with the same gRPC future as the underlying
	stream which will also be a :class:`grpc.Call`.
	"""

	def _on_call_done(self, future):
	for callback in self._callbacks:
	callback(future)

	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.

	# 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.

	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.

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

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

Conversation

parthea commented Mar 14, 2022 • edited Loading

tswast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parthea Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parthea commented Jun 30, 2022

tswast commented Jan 24, 2023

brianhlee commented Sep 19, 2023

parthea commented Sep 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parthea commented Oct 13, 2023

parthea commented Mar 14, 2022 •

edited

Loading

parthea Oct 13, 2023 •

edited

Loading