fix: Align Stream Closure Errors with Canonical API Behavior

[abbrowne126]

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

• Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
• Ensure the tests and linter pass
• Code coverage does not decrease (if any source code was changed)
• Appropriate docs were updated (if necessary)

Fixes #1415, #1176

[michaelpri10]

It seems that the reasoning for the use of Cancelled as a terminating stream error here is to prevent errors logs for what should be "clean" shutdowns (googleapis/google-cloud-python#8650 (comment)), so I wonder if this might cause exceptions like (googleapis/google-cloud-python#7826) to be thrown. Another worry is that we won't get exceptions for the new TERMINATING_STREAM_ERRORS, though I'm admittedly not certain how the mechanism actually works.

[abbrowne126]

I aligned this with the other client libs (ex Java, ex CPP) but this is a fair point - I believe these cancelled would actually be client initiated in this case so I'll keep it in terminating.

As for the exceptions/no exceptions, this will close the thread; this was already WAI for the library. Here's the details for ServiceConsumer (i.e. specific to our implementation):

• is_active is the condition to continue trying in thread_main's while loop
• is_active property is set by ResumableBidiRpc via the termination process
• The ResumableBidiRPC has a done callback, which will then be called - this method calls _shutdown
• When this is called, the rpc is destructed and the thread is shutdown
• This calls the close callback
• That close callback is passed to the StreamingPullFuture, which is how it's ultimately bubbled to the caller.

As for how the behavior works with termination, this is already covered in unit tests. Specific to the future, here's an example of where the error -> client-visible exception is tested

[michaelpri10]

Thanks for spelling out the details here, definitely makes a lot more sense now. This all looks good to me, just want to confirm if you've tested this with local changes and a reproduction of throwing an exception in BackgroundConsumer._thread_main.

[abbrowne126]

Yes - granted, the test involved mocking the ResumableBidiRpc but otherwise it was the same testing process. I saw the error bubbled to the future.

[michaelpri10]

Sample failures seem to be due to SMTs, so are unrelated to this PR.

[harshvardhanrajdan]

@abbrowne126 please update the status?