Deduplicate config refresh instead of fetch operation only (wip)

Author: adams85

src/ConfigServiceBase.ts

@@ -74,11 +74,16 @@ function nameOfConfigServiceStatus(value: ConfigServiceStatus): string {
   return ConfigServiceStatus[value] as string;
 }
 
+type ConfigRefreshOperation = {
+  promise: Promise<[FetchResult, ProjectConfig]>;
+  latestConfig: ProjectConfig;
+};
+
 export abstract class ConfigServiceBase<TOptions extends OptionsBase> {
   private status: ConfigServiceStatus;
 
   private pendingCacheSyncUp: Promise<ProjectConfig> | null = null;
-  private pendingFetch: Promise<FetchResult> | null = null;
+  private pendingConfigRefresh: ConfigRefreshOperation | null = null;
 
   protected readonly cacheKey: string;
 
@@ -119,25 +124,57 @@ export abstract class ConfigServiceBase<TOptions extends OptionsBase> {
   }
 
   protected async refreshConfigCoreAsync(latestConfig: ProjectConfig, isInitiatedByUser: boolean): Promise<[FetchResult, ProjectConfig]> {
-    const fetchResult = await this.fetchAsync(latestConfig);
-
-    let configChanged = false;
-    const success = fetchResult.status === FetchStatus.Fetched;
-    if (success
-        || fetchResult.config.timestamp > latestConfig.timestamp && (!fetchResult.config.isEmpty || latestConfig.isEmpty)) {
-      await this.options.cache.set(this.cacheKey, fetchResult.config);
+    let refreshOperation = this.pendingConfigRefresh;
 
-      configChanged = success && !ProjectConfig.equals(fetchResult.config, latestConfig);
-      latestConfig = fetchResult.config;
+    if (refreshOperation) {
+      const { promise, latestConfig: knownLatestConfig } = refreshOperation;
+      if (latestConfig.timestamp > knownLatestConfig.timestamp && (!latestConfig.isEmpty || knownLatestConfig.isEmpty)) {
+        refreshOperation.latestConfig = latestConfig;
+      }
+      return promise;
     }
 
-    this.onConfigFetched(fetchResult, isInitiatedByUser);
+    refreshOperation = { latestConfig } as ConfigRefreshOperation;
+    refreshOperation.promise = (async (refreshOperation: ConfigRefreshOperation) => {
+      const fetchResult = await this.fetchAsync(refreshOperation.latestConfig);
+
+      // NOTE: Further joiners may obtain more up-to-date configs from the external cache, and update
+      // operation.latestConfig before the operation completes, but those updates will be ignored.
+      // In other words, the operation may not return the most recent config obtained during its execution.
+      // However, this is acceptable, especially if we consider that reading and writing the external cache is
+      // not synchronized, which means that a more recent config can be overwritten with a stale one.
+      // (We don't make any effort to synchronize external cache access as that would be extremely hard,
+      // and we expect the "stuttering" resulting from this race condition to be temporary only.)
+      let { latestConfig } = refreshOperation;
+
+      const success = fetchResult.status === FetchStatus.Fetched;
+      if (success
+          || fetchResult.config.timestamp > latestConfig.timestamp && (!fetchResult.config.isEmpty || latestConfig.isEmpty)) {
+        await this.options.cache.set(this.cacheKey, fetchResult.config);
+
+        latestConfig = fetchResult.config;
+      }
 
-    if (configChanged) {
-      this.onConfigChanged(fetchResult.config);
-    }
+      return [fetchResult, latestConfig];
+    })(refreshOperation);
+
+    const refreshAndFinish = refreshOperation.promise
+      .finally(() => this.pendingConfigRefresh = null)
+      .then(refreshResult => {
+        const [fetchResult] = refreshResult;
 
-    return [fetchResult, latestConfig];
+        this.onConfigFetched(fetchResult, isInitiatedByUser);
+
+        if (fetchResult.status === FetchStatus.Fetched) {
+          this.onConfigChanged(fetchResult.config);
+        }
+
+        return refreshResult;
+      });
+
+    this.pendingConfigRefresh = refreshOperation;
+
+    return refreshAndFinish;
   }
 
   protected onConfigFetched(fetchResult: FetchResult, isInitiatedByUser: boolean): void {
@@ -150,12 +187,7 @@ export abstract class ConfigServiceBase<TOptions extends OptionsBase> {
     this.options.hooks.emit("configChanged", newConfig.config ?? new Config({}));
   }
 
-  private fetchAsync(lastConfig: ProjectConfig): Promise<FetchResult> {
-    return this.pendingFetch ??= this.fetchLogicAsync(lastConfig)
-      .finally(() => this.pendingFetch = null);
-  }
-
-  private async fetchLogicAsync(lastConfig: ProjectConfig): Promise<FetchResult> {
+  private async fetchAsync(lastConfig: ProjectConfig): Promise<FetchResult> {
     const options = this.options;
     options.logger.debug("ConfigServiceBase.fetchLogicAsync() called.");
 

src/ProjectConfig.ts

@@ -14,13 +14,6 @@ export class ProjectConfig {
 
   static readonly empty = new ProjectConfig(void 0, void 0, 0, void 0);
 
-  static equals(projectConfig1: ProjectConfig, projectConfig2: ProjectConfig): boolean {
-    // When both ETags are available, we don't need to check the JSON content.
-    return projectConfig1.httpETag && projectConfig2.httpETag
-      ? projectConfig1.httpETag === projectConfig2.httpETag
-      : projectConfig1.configJson === projectConfig2.configJson;
-  }
-
   constructor(
     readonly configJson: string | undefined,
     readonly config: Config | undefined,

test/helpers/fakes.ts

@@ -146,9 +146,11 @@ export class FakeConfigFetcherBase implements IConfigFetcher {
         this.config = fr.body ?? null;
         return fr;
       }
-      : () => this.config !== null
-        ? { statusCode: 200, reasonPhrase: "OK", eTag: this.getEtag(), body: this.config } as IFetchResponse
-        : { statusCode: 404, reasonPhrase: "Not Found" } as IFetchResponse;
+      : () => {
+        return this.config === null ? { statusCode: 404, reasonPhrase: "Not Found" } as IFetchResponse
+          : this.getEtag() === lastEtag ? { statusCode: 304, reasonPhrase: "Not Modified" } as IFetchResponse
+          : { statusCode: 200, reasonPhrase: "OK", eTag: this.getEtag(), body: this.config } as IFetchResponse;
+      };
 
     await delay(this.callbackDelay);