-
Notifications
You must be signed in to change notification settings - Fork 46
Add flight-controlled thread pool expansion for silent requests, Fixes AB#3492151 #2865
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
…92151 Fixes: AB#3492151 Problem: The Broker's silent token request thread pool (sSilentExecutor) is fixed at 5 threads. Under high load scenarios with multiple concurrent ATS requests from various client apps (Teams, Outlook, OneAuth), this can cause request queuing, increased latency, and potential timeouts. Solution: Introduce a flight-controlled mechanism to expand the thread pool from 5 to 8 threads, applying ONLY to the Broker module (not MSAL client apps). Changes: - CommonFlight.java: Add ENABLE_EXPANDED_BROKER_SILENT_THREAD_POOL flight (default: false) - CommandDispatcher.java: Add initializeSilentExecutorForBroker() and resetSilentRequestExecutorWithSize() methods with graceful executor shutdown handling - AndroidBrokerFlightsManager.kt: Call initializeBrokerSilentExecutor() during Broker initialization after flights are loaded Key design decisions: - Uses Android process isolation: Broker runs in separate process, so static sSilentExecutor is independent from MSAL client apps - Broker-only initialization: AndroidBrokerFlightsManager is Broker-specific code that MSAL apps never call - Flight-controlled: Safe rollout via ECS with instant rollback capability - Graceful shutdown: Existing executor is properly terminated before creating expanded pool
|
✅ Work item link check complete. Description contains link AB#3492151 to an Azure Boards work item. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces a flight-controlled mechanism to expand the Broker's silent token request thread pool from 5 to 8 threads. The change is Broker-specific and leverages Android process isolation to ensure MSAL client apps remain unaffected. The expansion is controlled by the ENABLE_EXPANDED_BROKER_SILENT_THREAD_POOL flight with a default value of false.
Changes:
- Added new flight enum
ENABLE_EXPANDED_BROKER_SILENT_THREAD_POOLfor controlling thread pool expansion - Implemented
initializeSilentExecutorForBroker()andresetSilentRequestExecutorWithSize()methods to dynamically resize the silent executor based on flight state - Updated changelog to reflect the thread pool size increase
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| common4j/src/main/com/microsoft/identity/common/java/flighting/CommonFlight.java | Adds flight enum for enabling expanded broker silent thread pool with appropriate documentation |
| common4j/src/main/com/microsoft/identity/common/java/controllers/CommandDispatcher.java | Implements broker-specific executor initialization with graceful shutdown and flight-based pool sizing logic |
| changelog.txt | Documents thread pool size increase (though entry needs more context about flight-controlled nature) |
common4j/src/main/com/microsoft/identity/common/java/controllers/CommandDispatcher.java
Outdated
Show resolved
Hide resolved
common4j/src/main/com/microsoft/identity/common/java/controllers/CommandDispatcher.java
Outdated
Show resolved
Hide resolved
common4j/src/main/com/microsoft/identity/common/java/controllers/CommandDispatcher.java
Outdated
Show resolved
Hide resolved
common4j/src/main/com/microsoft/identity/common/java/controllers/CommandDispatcher.java
Show resolved
Hide resolved
common4j/src/main/com/microsoft/identity/common/java/controllers/CommandDispatcher.java
Show resolved
Hide resolved
common4j/src/main/com/microsoft/identity/common/java/controllers/CommandDispatcher.java
Outdated
Show resolved
Hide resolved
common4j/src/main/com/microsoft/identity/common/java/flighting/CommonFlight.java
Outdated
Show resolved
Hide resolved
common4j/src/main/com/microsoft/identity/common/java/controllers/CommandDispatcher.java
Outdated
Show resolved
Hide resolved
common4j/src/main/com/microsoft/identity/common/java/controllers/CommandDispatcher.java
Show resolved
Hide resolved
|
Just curious, how did we arrive at the number for threads allowed for silent requests as 8? Is it because we did some stress testing and confirmed that it is performing better? Or are we going to try it out in PROD directly? |
The count is based on experiment being done on client side(WXP) thread pool size. For context the flow is: So,
|
common4j/src/main/com/microsoft/identity/common/java/controllers/CommandDispatcher.java
Show resolved
Hide resolved
common4j/src/main/com/microsoft/identity/common/java/controllers/CommandDispatcher.java
Outdated
Show resolved
Hide resolved
| * <p> | ||
| * This method should ONLY be called by Broker during its initialization phase | ||
| * when the flight check determines that expanded pool is enabled. | ||
| * MSAL client apps should NOT call this method - they will use the default pool size. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you moved the flight check to broker in this iteration, if you want to avoid MSAL clients to avoid calling this method (in spite of the very clear Javadoc), you can use throwIfNotInvokedByBroker method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Based on my code analysis, throwIfNotInvokedByBroker is part of BrokerHostingAppPublicApi
- My PR change initializeSilentExecutorWithExpandedPool() is in common module
Common module cannot referrer Broker API.
I understand the problem identified here that any non-Broker module ex: MSAL can call this API and change the thread pool size. I would like to take others thought, if we need more protection here or java doc is sufficient for now.
vis: @siddhijain
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is good to have that protection in place - Check the calling package and throw exception if anyone other than the Broker calls this method. If the javadoc is not read (which is generally the case), we won't even know if the MSAL clients start using thread pool size of 8.
common4j/src/main/com/microsoft/identity/common/java/controllers/CommandDispatcher.java
Show resolved
Hide resolved
| } | ||
|
|
||
| if (!terminated) { | ||
| Logger.error(methodTag, "Executor did not fully terminate. Creating new executor anyway.", null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also what happens here if there are 2 Executors now? 1 with the thread pool size of 5 and another one with a size of 8?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's only one static reference sSilentExecutor so we can't have two executors being actively used simultaneously. When we create a new executor at line 965, the old reference is replaced.
However, if we reach line 960-962 (executor didn't fully terminate), the old executor's threads may still be running in the background temporarily until their tasks complete or they're garbage collected. This is an edge case that:
- Should rarely occur - we wait 500ms for graceful shutdown, then call shutdownNow() which interrupts threads, then wait another 1000ms
- Has minimal impact - old tasks complete on old threads, new tasks use new executor
- Self-resolves - old executor is garbage collected once threads complete
This scenario is also no worse than before this PR, where resetSilentRequestExecutor() simply created a new executor without any shutdown, potentially leaving the old executor running indefinitely.
Fixes: AB#3492151
Problem:
The Broker's silent token request thread pool (sSilentExecutor) is fixed at 5 threads. Under high load scenarios with multiple concurrent ATS requests from various client apps (Teams, Outlook, OneAuth), this can cause request queuing, increased latency, and potential timeouts.
Solution:
Introduce a flight-controlled mechanism to expand the thread pool from 5 to 8 threads, applying ONLY to the Broker module (not MSAL client apps).
Changes:
Key design decisions: