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

PubSub: Propagate subscribe callback errors to main thread #7954

Merged
merged 1 commit into from
May 15, 2019

Conversation

plamut
Copy link
Contributor

@plamut plamut commented May 13, 2019

Closes #7917.

This PR implements a user request to propagate exceptions from streaming pull callbacks to the main thread awaiting the result of the future returned by the subscriber.subscribe() call.

How to test

Steps to reproduce:

  • Prerequsite: Have a working service account, pubsub topic, and subscription created.
  • Publish a message to the topic.
  • Run the sample subscriber code posted under the issue (adjust subscription name as necessary).

Actual result (before the fix):

  • Nothing happens in the main thread. The exception is raised in the callback, but the main thread keeps waiting for for fut.result().

Expected result (after the fix):

  • An exception from the callback is propagated to fut.result(), and the main thread exists almost immediately after the callback is invoked.

@plamut plamut added the api: pubsub Issues related to the Pub/Sub API. label May 13, 2019
@plamut plamut requested a review from crwilcox as a code owner May 13, 2019 16:21
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 13, 2019
@@ -299,21 +300,26 @@ def heartbeat(self):
if self._rpc is not None and self._rpc.is_active:
self._rpc.send(types.StreamingPullRequest())

def open(self, callback):
def open(self, callback, on_callback_error):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW, this method signature change is safe, as StreamingPullManager class is only used internally in the subscriber client's subscribe() method, and is not mentioned anywhere in the user docs, at least from what I can see.

@sduskis sduskis requested a review from a team May 14, 2019 18:24
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.

PubSub: Subscriber future not raising exception that happen in callbacks
3 participants