search

googleapis / google-cloud-cpp

C++ Client Libraries for Google Cloud Services
https://cloud.google.com/
Apache License 2.0
550 stars 372 forks source link

Stubs reuse `grpc.channel_id` #13328

Closed dbolduc closed 10 months ago

dbolduc commented 10 months ago

I noticed this when refactoring Bigtable to pull the GrpcAuthenticationStrategy creation outside of the stub factory. Using a mock revealed that we were setting every grpc.channel_id to 0. The whole point of setting the arg is to give each channel a unique value, so they use a distinct channel pool.

https://github.com/googleapis/google-cloud-cpp/blob/11fd85cb6ad31a07c6025ac5254e3ffd7ebe2922/google/cloud/bigtable/internal/bigtable_stub_factory.cc#L68-L70

This lambda needs to be mutable. We also need to fix this in pubsub and storage.

coryan commented 10 months ago

I am sure there is a bug, but not the mutable thing:

https://godbolt.org/z/q4rMnb65a

id is captured by reference, the reference is not changing (it can't, but that is neither here nor there).

dbolduc commented 10 months ago

Hm.... I have a test that passes with or without the mutable, and I see your godbolt link....

I probably:

  1. wrote a bad test
  2. added mutable to the lambda
  3. fixed the test and didn't realize.

and thought 2 was the real fix.

I am sure there is a bug

Probably not. :unamused: