impl(oauth2): add BackgroundCredentials decorator and update tests#16159
impl(oauth2): add BackgroundCredentials decorator and update tests#16159scotthart wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces BackgroundCredentials, a decorator class designed to manage the lifetime of BackgroundThreads used by child credentials, preventing thread join-on-self issues. It refactors RegionalAccessBoundaryTokenManager to accept a completion queue (RestPureCompletionQueue) instead of managing its own background threads, and updates associated tests and integration tests accordingly. Feedback was provided regarding BackgroundCredentials::project_id(Options const&), which currently ignores the passed Options parameter and should instead delegate to the child's overload that accepts options.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16159 +/- ##
==========================================
- Coverage 92.20% 92.20% -0.01%
==========================================
Files 2264 2265 +1
Lines 209084 209113 +29
==========================================
+ Hits 192791 192815 +24
- Misses 16293 16298 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
alvarowolfx
left a comment
There was a problem hiding this comment.
It seems good to me, just not sure about what the AI found on passing the Options on project_id call. Some questions that I added might be related to my poor knowledge of async stuff in CPP
5ec2e29 to
dfa2551
Compare
This PR moves the BackgroundThreads member from the RABTokenManager to a parent decorator, and the RABTokenManager is constructed with a CompletionQueue from the parent decorator BackgroundThreads.
This is all necessary to avoid a situation where a thread in the pool could attempt to destroy the BackgroundThreads object. As ~BackgroundThreads joins all threads in the pool, this creates an opportunity for a thread in the pool to attempt to join itself, which is an error.
The GCS test is also updated to eliminate flakiness introduced by the RAB functionality.