Avoid calling asynchronous requests from setInterval() #3476
Description
Core Library
@azure/msal-node
Core Library Version
Latest
Wrapper Library
Not Applicable
Wrapper Library Version
Latest
Description
Hello team! I'm reporting a bug which happens in the odd case that a network request might take longer than an interval. In such cases, users will experience their Node process not to close properly, and errors might not be able to bubble up appropriately.
This issue happens because acquireTokenWithDeviceCode( has a setInterval that triggers an async function in this line: msal-common/src/client/DeviceCodeClient.ts#L156. The interval is triggered with a duration of pollingIntervalMilli.
The function sent to that setInterval is an asynchronous function that calls and waits for executePostToTokenEndpoint(.
It could be the case that executePostToTokenEndpoint( can take longer than the interval's duration. In which case, setInterval will trigger the asynchronous function again, since setInterval does not wait for the internal function to finish.
I am aware that pollingIntervalMilli is set to be as many seconds as deviceCodeResponse.interval says (source link). In the cases I've been able to test, this means 5 seconds. Depending on the network speed and the availability of the services, executePostToTokenEndpoint( might not be fulfilled on time.
For that acquireTokenWithDeviceCode(, this could be fixed by using an for loop, similar to:
// Assuming the for loop is within an async function, of course
private async acquireTokenWithDeviceCode(
/* ... */
// Instead of returning a promise and calling to setInterval inside of that promise...
for (let i = 0; i <= maxIterations; i++) {
if (request.cancel) {
/* ... */
} else if (userSpecifiedTimeout && userSpecifiedTimeout < deviceCodeExpirationTime && TimeUtils.nowSeconds() > userSpecifiedTimeout) {
/* ... */
} else if (TimeUtils.nowSeconds() > deviceCodeExpirationTime) {
/* ... */
} else {
/* ... */
const response = await this.executePostToTokenEndpoint(/* ... */);
if (response.body && response.body.error === Constants.AUTHORIZATION_PENDING) {
this.logger.info(response.body.error_description || "no_error_description");
} else {
return response.body;
}
// Here or somewhere near this we would wait some time.
// In this example we use a new function: delay
// The for loop won't start again before this promise is resolved.
await delay(pollingIntervalMilli);
}
}
}For the code above to work, delay could be defined as follows:
function delay<T>(t: number, value?: T): Promise<T | void> {
return new Promise((resolve) => setTimeout(() => resolve(value), t));
}This issue is not blocking us.
Error Message
No response
Msal Logs
No response
MSAL Configuration
Not applicable.Relevant Code Snippets
See the description.Reproduction Steps
To reproduce this one would need to mock the HTTP layer to artificially introduce a delay longer than the interval duration.
Expected Behavior
The polling of the device code endpoint should not happen before the interval finishes.
Identity Provider
Other
Browsers Affected (Select all that apply)
Other
Regression
No response
Source
Internal (Microsoft)