Avoid multiple refresh

[zhiyuanliang-ms]

Why this PR?

class Timer {
    #backoffEnd; // timestamp
    #interval;

    constructor(interval) {
        this.#interval = interval;
        this.#backoffEnd = Date.now();
    }

    canRefresh() {
        return Date.now() >= this.#backoffEnd;
    }

    reset() {
        this.#backoffEnd = Date.now() + this.#interval;
    }
}

async function refresh() {
    await f();
}

async function f() {
    if (!timer.canRefresh()) {
        console.log('timer rejects')
        return Promise.resolve(false);
    }
    console.log('f starts')
    await new Promise(resolve => setTimeout(resolve, 1000))
    console.log('f ends')
    timer.reset()
}

for (let i = 0; i < 3; i++) {
    refresh();
}

The above code snippet simulate the current implementation of refresh and it will produce the following output

f starts
f starts
f starts
f ends
f ends
f ends

The timer fails to prevent refresh operation from being executed multiple times during refresh interval.

Solution

Except for the web worker scenario, javascript is in general single thread. For an async function, it will be synchronously executed until an await occurs, then it will give the control to the event loop. So this should be atomic. So the solution is to maintain a boolean variable refreshInProgress to indicate whether there is any ongoing refresh call. The check/setting of the flag will be done synchronously so it should be concurrent/"thread" safe.

[RichardChen820]

Do we really need to address concurrency issue in this method? The refresh method's job is very simple, attempt to refresh config once, the concurrency issue really need to be considered by the caller depends on how the caller use it.

[RichardChen820]

If we think it‘s of great value resolving it in the provider, I would rather like to have a new method something like ExclusiveRefresh to address it.

[Eskibear]

the concurrency issue really need to be considered by the caller depends on how the caller use it.

That makes sense. Then this task can simply become to add some details to the API mentioning RefreshTimer, to make sure customers don't have the gap about the behavior.

AppConfiguration-JavaScriptProvider/src/AzureAppConfiguration.ts

Lines 6 to 10 in 0027254

	export type AzureAppConfiguration = {
	/**
	* API to trigger refresh operation.
	*/
	refresh(): Promise<void>;

[juniwang]

The latest code is much simpler, I like it!