No region tags are edited in this PR.

This comment is generated bysnippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, addsnippet-bot:force-run label or use the checkbox below:

• Refresh this comment

Looking at Firestore vs. Pub/Sub, I see Firestore is passing the method in thegRPC transport and Pub/Sub is passing the method in theGAPIC client.

I wonder if that contributed to the behavior difference between the two that requiresthis workaround? (Also see:googleapis/python-pubsub#93 (comment) and#30)

@jameslynnwu For it "not working" whenLoggingServiceV2Client().tail_log_entries was passed did it hang, error, or something else?

Great find!
When I pass inLoggingServiceV2Client().tail_log_entries, it doesn't respond and the request does not seem to be initiated.

However, if I use the same workaround, it works.

# Does not respond. Request doesn't seem to be initiatedclient = LoggingServiceV2Client()self.tail_stub = bidi.BidiRpc(client.logging.tail_log_entries)# Worksclient = LoggingServiceV2Client()self.tail_stub = bidi.BidiRpc(client.logging.transport.tail_log_entries)# Worksclient = LoggingServiceV2Client()client.transport._prefetch_first_result_ = Falseself.tail_stub = bidi.BidiRpc(client.logging.tail_log_entries)

The Pub/Sub client passes thestreaming_pull() method when creating aBidi instance. This method comes from the auto-generated client andinvokes the wrapped transport method.

The subscriber client does not pass any metadata on its own toResumableBidiRpc, it just leaves it up to the (generated) transport to do thewrapping, which setsclient_info on requests, among other things.

As for the changes in this PR - since no metadata is passed toBidiRpc, the default valueNone is used for it.

Before the PR,BidiRpc.open() called the following (sendingNone for metadata:

call=self._start_rpc(iter(request_generator),metadata=self._rpc_metadata)

With the changes in this PR, however, that changes:

ifself._rpc_metadata:call=self._start_rpc(iter(request_generator),metadata=self._rpc_metadata)# use metadata from self._start_rpc if no other metadata is specifiedelse:call=self._start_rpc(iter(request_generator))

Since metadata isNone, this code now effectively becomes the following:

call=self._start_rpc(iter(request_generator))

No explicitNone metadata is passed in, thus the generatedstreaming_pull() method now usesits own default, which is an empty tuple.

When the (wrapped) method is eventually called, thefollowing logic in_GapicCallable determines the metadata:

ifself._metadataisnotNone:metadata=kwargs.get("metadata", [])# Due to the nature of invocation, None should be treated the same# as not specified.ifmetadataisNone:metadata= []metadata=list(metadata)    ...

If I traced this correctly, there should be no difference between passingNone and an empty tuple as a kwarg. Both get transformed to an empty list, and the original metadata from the method wrapping time is not modified.

Does this answer the question?

As for passing in a wrapped vs. un-wrapepd method - the regression caused by a Firestore fix back then seems to only affect the wrapped methods, yes, as the key changelives in in the_StreamingResponseIterator (the latter wraps the stream errors, and only comes into play with the wrapped methods).

We had to introduce a flag that changes how_StreamingResponseIterator behaves, as Firestore's fix can break Pub/Sub. :)
(and Logging, if I understood@jameslynnwu correctly).

One more thing to note, though we probably not want to address all of this at once.

Passing in theclient.logging.tail_log_entries vsclient.logging.transport.tail_log_entries changes what is passed into theshould_recover method ofResumableBidiRPC from either agrpc.Call or agoogle.api_core.exceptions.GoogleAPICallError to agrpc.Call, respectively.

If we can ensure that onlyclient.logging.tail_log_entries is passed it that would makeshould_recover more predictable. Alternatively, we should update the code to always pass the same result intoshould_recover regardless of ifclient.logging.tail_log_entries orclient.logging.transport.tail_log_entries is passed in.

Also, it's interesting thatshould_recover is called twice from callbacks and from exception.

@plamut Thank you for the detailed analysis. 🙏

I'm OOO the rest of the week so I'll come back and look at this next week.

As@plamut explained above, this PR is a no-op, if you pass the wrapped method the metadata will get added (which is why Pub/Sub doesn't passmetadata explicitly tostart_rpc).

I had a bit of trouble tracing this through but was able to confirm the expected headers in the gRPC logs:

export GRPC_TRACE=allexport GRPC_VERBOSITY=debug

I0625 21:17:38.217348925 2619277 chttp2_transport.cc:1363]   HTTP:0:HDR:CLI: :scheme: httpsI0625 21:17:38.217356118 2619277 chttp2_transport.cc:1363]   HTTP:0:HDR:CLI: :method: POSTI0625 21:17:38.217363271 2619277 chttp2_transport.cc:1363]   HTTP:0:HDR:CLI: :authority: pubsub.googleapis.com:443I0625 21:17:38.217370411 2619277 chttp2_transport.cc:1363]   HTTP:0:HDR:CLI: :path: /google.pubsub.v1.Subscriber/StreamingPullI0625 21:17:38.217377702 2619277 chttp2_transport.cc:1363]   HTTP:0:HDR:CLI: x-goog-api-client: gl-python/3.8.3 grpc/1.38.0 gax/1.30.0 gccl/2.5.0