Skip to content

impl(oauth2): add BackgroundCredentials decorator and update tests#16159

Open
scotthart wants to merge 2 commits into
googleapis:mainfrom
scotthart:oauth2_rab_thread_pool_lifetime
Open

impl(oauth2): add BackgroundCredentials decorator and update tests#16159
scotthart wants to merge 2 commits into
googleapis:mainfrom
scotthart:oauth2_rab_thread_pool_lifetime

Conversation

@scotthart

Copy link
Copy Markdown
Member

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.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread google/cloud/internal/oauth2_background_credentials.h Outdated
@codecov

codecov Bot commented Jun 15, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 89.28571% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.20%. Comparing base (e908d88) to head (5ec2e29).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
...gle/cloud/internal/oauth2_background_credentials.h 72.72% 6 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@alvarowolfx alvarowolfx changed the title impl(ouath2): add BackgroundCredentials decorator and update tests impl(oauth2): add BackgroundCredentials decorator and update tests Jun 15, 2026

@alvarowolfx alvarowolfx left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread google/cloud/internal/oauth2_background_credentials.h Outdated
Comment thread google/cloud/internal/oauth2_background_credentials.h
@scotthart scotthart force-pushed the oauth2_rab_thread_pool_lifetime branch from 5ec2e29 to dfa2551 Compare June 15, 2026 17:59
@scotthart scotthart closed this Jun 15, 2026
@scotthart scotthart reopened this Jun 15, 2026
@alvarowolfx alvarowolfx self-requested a review June 15, 2026 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants