Fix failure of calling authenticated APIs from secondary AsgardeoProvider Instances (Multi-Provider Support)#337
Conversation
d3e913b to
0744686
Compare
packages/browser/src/__legacy__/http-client/clients/axios-http-client.ts
Show resolved
Hide resolved
a6c1841 to
12fd275
Compare
DonOmalVindula
left a comment
There was a problem hiding this comment.
The multiton refactoring looks good overall and the approach is correct. However, there's a this-binding bug that needs to be fixed before merging — see inline comments.
packages/browser/src/__legacy__/http-client/clients/axios-http-client.ts
Show resolved
Hide resolved
packages/browser/src/__legacy__/http-client/clients/axios-http-client.ts
Outdated
Show resolved
Hide resolved
DonOmalVindula
left a comment
There was a problem hiding this comment.
A few more issues I noticed during review.
|
|
||
| let _getSignOutURLFromSessionStorage: boolean = false; | ||
|
|
||
| const _httpClient: HttpClientInstance = HttpClient.getInstance(); |
There was a problem hiding this comment.
Bug: worker-core.ts has the same pattern at the same line — HttpClient.getInstance() without an instanceId — but was missed in this PR. The web worker path will always fall back to instanceId=0 regardless of which provider spawned it.
Suggestion: Thread the instanceID through to WebWorkerCore the same way it's done in MainThreadClient:
export const WebWorkerCore = async (
instanceID: number,
config: AuthClientConfig<WebWorkerClientConfig>,
...
): Promise<WebWorkerCoreInterface> => {
...
const _httpClient: HttpClientInstance = HttpClient.getInstance(instanceID);
...
};See packages/browser/src/__legacy__/worker/worker-core.ts:65.
There was a problem hiding this comment.
Resolved this
| private static isHandlerEnabled: boolean; | ||
| private static instances: Map<number, HttpClientInstance> = new Map(); | ||
| private static clientInstances: Map<number, HttpClient> = new Map(); | ||
| private isHandlerEnabled: boolean = true; |
There was a problem hiding this comment.
Memory leak: Instances are added to these static Maps but never removed. If an AsgardeoProvider unmounts and remounts (e.g. React hot-reload, dynamic multi-tenant switching), stale axios instances with outdated interceptors and callbacks will accumulate.
Suggestion: Add a static cleanup method:
public static destroyInstance(instanceId: number): void {
const axiosInstance = this.instances.get(instanceId);
if (axiosInstance) {
// Eject interceptors to prevent memory leaks
axiosInstance.interceptors.request.clear();
axiosInstance.interceptors.response.clear();
}
this.instances.delete(instanceId);
this.clientInstances.delete(instanceId);
}This should be called during provider teardown.
There was a problem hiding this comment.
Resolved this
| private static clientInstance: HttpClient; | ||
| private static isHandlerEnabled: boolean; | ||
| private static instances: Map<number, HttpClientInstance> = new Map(); | ||
| private static clientInstances: Map<number, HttpClient> = new Map(); |
There was a problem hiding this comment.
Dead code: clientInstances is write-only — nothing ever calls clientInstances.get(). If you add a destroyInstance() method (see my other comment), this map becomes useful for cleanup. Otherwise, it should be removed to avoid confusion.
There was a problem hiding this comment.
Resolved this
…ct 'this' context in HttpClient
…e to support instance ID - Implement `destroyInstance` method in `HttpClient` to clean up instances and prevent memory leaks. - Update `WebWorkerCore` to accept an `instanceID` parameter for better instance management. - Modified `workerReceiver` to handle instance ID during initialization and pass it to `WebWorkerCore`.
758ffe6 to
74bdd93
Compare
🦋 Changeset detectedThe changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. |
DonOmalVindula
left a comment
There was a problem hiding this comment.
The core singleton → multiton refactoring looks correct and all previous review comments have been addressed. A few remaining items to clean up before merging.
| if (axiosInstance) { | ||
| // Eject interceptors to prevent memory leaks | ||
| axiosInstance.interceptors.request.clear(); | ||
| axiosInstance.interceptors.response.clear(); |
There was a problem hiding this comment.
destroyInstance() is defined but never called anywhere. It needs to be wired into the provider teardown/unmount lifecycle (e.g., in AsgardeoProvider's cleanup) to actually prevent the memory leak it was designed to address. Without a caller, this is dead code.
Please either:
- Wire it into the provider's
useEffectcleanup, or - Remove it and track the cleanup as a separate follow-up issue.
|
|
||
| const message: Message<AuthClientConfig<WebWorkerClientConfig>> = { | ||
| data: config, | ||
| const message: Message<AuthClientConfig<WebWorkerClientConfig> & {instanceID: number}> = { |
There was a problem hiding this comment.
Mixing instanceID into the config data payload via intersection type (& {instanceID: number}) is fragile. If AuthClientConfig ever adds an instanceID field, it will silently conflict.
Nit: Consider passing instanceID as a separate field on the Message type rather than stuffing it into data.
| return this.axiosInstance; | ||
| public static getInstance(instanceId: number = 0): HttpClientInstance { | ||
| if (this.instances.has(instanceId)) { | ||
| return this.instances.get(instanceId)!; |
There was a problem hiding this comment.
Nit: Naming inconsistency — this method uses instanceId (camelCase) while MainThreadClient, WebWorkerCore, and workerReceiver all use instanceID (all-caps ID). Pick one convention and use it consistently across the codebase.
|
You can address the added comments in a follow-up PR |
Purpose
This pull request introduces multi-HTTP clients support when multiple provider instances are implemented. This ensures that all authenticated HTTP operations are isolated per instance.
The most important changes are:
Instance-specific HTTP Client Management
HttpClientto support multiple isolated HTTP client/axios instances, each keyed by instance ID, preventing state conflicts between auth contexts. All HTTP handler flags and callbacks are now instance-specific. [1] [2] [3] [4] [5] [6] [7] [8]HttpClient.getInstance()to provide the correct instance ID, ensuring HTTP requests are routed through the correct context. [1] [2] [3] [4]Related Issues
Related PRs
Checklist
Security checks