-
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
Pub/Sub has no way to track errors from the subscriber thread. #3888
Comments
I think I agree with this but it is worth asking: Is there any situation where there are multiple exceptions? Futures can only have single resolutions. I do think it is fine to have a future that never has a result (or only gets one if you explicitly close), but I am operating under the assumption that when the exception occurs, we will cause that thread to exit and set the exception on the future. |
I think that any exception that bubbles to the top of the consumer thread
is a thread ending exception, so there should be only one.
The rest of your comment matches my understanding.
…On Mon, Aug 28, 2017, 10:07 PM Luke Sneeringer ***@***.***> wrote:
I *think* I agree with this but it is worth asking: Is there any
situation where there are multiple exceptions? Futures can only have single
resolutions.
I do think it is fine to have a future that never has a result (or only
gets one if you explicitly close), but I am operating under the assumption
that when the exception occurs, we will cause that thread to exit and set
the exception on the future.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#3888 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAPUczQJq3yTWzMB1tJG97X3U-w8WS_cks5sc5yLgaJpZM4PE4py>
.
|
A workaround would be to override the default Policy used by the Client. For instance: class Custom(Policy):
def on_exception(self, exception):
try:
return super(Custom, self).on_exception(exception)
except:
# handle consumer exceptions here, for instance reject a promise or
# set threading.Event to communicate the failure to the main thread.
raise |
Having a future to check on would be a whole lot cleaner probably. At least it would be clear that control was being passed around, whereas having the on_exception() is vague and requires understanding the internals some. We've had to do something similar because we see this error which never seems to recover (running in google container engine):
I'm inclined to believe that a connection broke or shutdown, causing a socket to trigger an "OS Error". To work around the wedging, our code does something like this. We set a variable to indicate to the outer loop that the client might be bad.
then our listener can tell when we've hit an exception that would wedge our server, and instead it restarts the client:
That said, I'm not sure if we should add some mutexes here and be more thread-safe. Also this may have other issues since it's unclear if the old client ever shuts down cleanly. There's nothing in the API docs about it, and python can be lazy about this kind of resource cleanup, especially since worker threads may linger with a reference to the old client object. https://googlecloudplatform.github.io/google-cloud-python/latest/pubsub/publisher/api/client.html |
Presently if you subscribe and an error occurs in the consumer's thread there's no way to respond to the error on the main thread.
It just logs the exception and hangs:
It seems that
consumer.open
should return a future. This future could be used to both block the main thread to consume message and pass through exceptions.So the current sample:
Could instead be:
The text was updated successfully, but these errors were encountered: