From c18a7cb3e82f910426c1185401a885bfa2c0f4d8 Mon Sep 17 00:00:00 2001 From: Justin Baur <19896123+justindbaur@users.noreply.github.com> Date: Mon, 26 Aug 2024 14:23:33 -0400 Subject: [PATCH] [PM-8748] Ensure Reasonable ConfigService Emission (#10452) * Add Slow Emission Guard * Remove Jest Timer Call * Do Slow Emission Guard For All Environments * Update Comment --- .../services/config/config.service.spec.ts | 66 +++++++++++++++++-- .../services/config/default-config.service.ts | 45 ++++++++----- 2 files changed, 88 insertions(+), 23 deletions(-) diff --git a/libs/common/src/platform/services/config/config.service.spec.ts b/libs/common/src/platform/services/config/config.service.spec.ts index d7e33473d01..d03103f255c 100644 --- a/libs/common/src/platform/services/config/config.service.spec.ts +++ b/libs/common/src/platform/services/config/config.service.spec.ts @@ -3,8 +3,8 @@ * @jest-environment ../../libs/shared/test.environment.ts */ -import { mock } from "jest-mock-extended"; -import { Subject, firstValueFrom, of } from "rxjs"; +import { matches, mock } from "jest-mock-extended"; +import { BehaviorSubject, Subject, bufferCount, firstValueFrom, of } from "rxjs"; import { FakeGlobalState, @@ -35,6 +35,7 @@ import { RETRIEVAL_INTERVAL, GLOBAL_SERVER_CONFIGURATIONS, USER_SERVER_CONFIG, + SLOW_EMISSION_GUARD, } from "./default-config.service"; describe("ConfigService", () => { @@ -65,12 +66,14 @@ describe("ConfigService", () => { describe.each([null, userId])("active user: %s", (activeUserId) => { let sut: DefaultConfigService; + const environmentSubject = new BehaviorSubject(environmentFactory(activeApiUrl)); + beforeAll(async () => { await accountService.switchAccount(activeUserId); }); beforeEach(() => { - environmentService.environment$ = of(environmentFactory(activeApiUrl)); + environmentService.environment$ = environmentSubject; sut = new DefaultConfigService( configApiService, environmentService, @@ -129,7 +132,8 @@ describe("ConfigService", () => { await firstValueFrom(sut.serverConfig$); expect(logService.error).toHaveBeenCalledWith( - `Unable to fetch ServerConfig from ${activeApiUrl}: Unable to fetch`, + `Unable to fetch ServerConfig from ${activeApiUrl}`, + matches((e) => e.message === "Unable to fetch"), ); }); }); @@ -138,6 +142,10 @@ describe("ConfigService", () => { const response = serverConfigResponseFactory(); const newConfig = new ServerConfig(new ServerConfigData(response)); + beforeEach(() => { + configApiService.get.mockResolvedValue(response); + }); + it("should be a new config", async () => { expect(newConfig).not.toEqual(activeUserId ? userStored : globalStored[activeApiUrl]); }); @@ -149,8 +157,6 @@ describe("ConfigService", () => { }); it("returns the updated config", async () => { - configApiService.get.mockResolvedValue(response); - const actual = await firstValueFrom(sut.serverConfig$); // This is the time the response is converted to a config @@ -270,6 +276,51 @@ describe("ConfigService", () => { }); }); }); + + describe("slow configuration", () => { + const environmentSubject = new BehaviorSubject(null); + + let sut: DefaultConfigService = null; + + beforeEach(async () => { + const config = serverConfigFactory("existing-data", tooOld); + environmentService.environment$ = environmentSubject; + + globalState.stateSubject.next({ [apiUrl(0)]: config }); + userState.stateSubject.next([userId, config]); + + configApiService.get.mockImplementation(() => { + return new Promise((resolve) => { + setTimeout(() => { + resolve(serverConfigResponseFactory("slow-response")); + }, SLOW_EMISSION_GUARD + 20); + }); + }); + + sut = new DefaultConfigService( + configApiService, + environmentService, + logService, + stateProvider, + authService, + ); + }); + + afterEach(() => { + jest.resetAllMocks(); + }); + + it("emits old configuration when the http call takes a long time", async () => { + environmentSubject.next(environmentFactory(apiUrl(0))); + + const configs = await firstValueFrom(sut.serverConfig$.pipe(bufferCount(2))); + + await jest.runOnlyPendingTimersAsync(); + + expect(configs[0].gitHash).toBe("existing-data"); + expect(configs[1].gitHash).toBe("slow-response"); + }); + }); }); function apiUrl(count: number) { @@ -305,8 +356,9 @@ function serverConfigResponseFactory(hash?: string) { }); } -function environmentFactory(apiUrl: string) { +function environmentFactory(apiUrl: string, isCloud: boolean = true) { return { getApiUrl: () => apiUrl, + isCloud: () => isCloud, } as Environment; } diff --git a/libs/common/src/platform/services/config/default-config.service.ts b/libs/common/src/platform/services/config/default-config.service.ts index 16878a72832..74dd5055d4b 100644 --- a/libs/common/src/platform/services/config/default-config.service.ts +++ b/libs/common/src/platform/services/config/default-config.service.ts @@ -24,7 +24,7 @@ import { UserId } from "../../../types/guid"; import { ConfigApiServiceAbstraction } from "../../abstractions/config/config-api.service.abstraction"; import { ConfigService } from "../../abstractions/config/config.service"; import { ServerConfig } from "../../abstractions/config/server-config"; -import { EnvironmentService, Region } from "../../abstractions/environment.service"; +import { Environment, EnvironmentService, Region } from "../../abstractions/environment.service"; import { LogService } from "../../abstractions/log.service"; import { devFlagEnabled, devFlagValue } from "../../misc/flags"; import { ServerConfigData } from "../../models/data/server-config.data"; @@ -34,6 +34,8 @@ export const RETRIEVAL_INTERVAL = devFlagEnabled("configRetrievalIntervalMs") ? (devFlagValue("configRetrievalIntervalMs") as number) : 3_600_000; // 1 hour +export const SLOW_EMISSION_GUARD = 800; + export type ApiUrl = string; export const USER_SERVER_CONFIG = new UserKeyDefinition(CONFIG_DISK, "serverConfig", { @@ -64,29 +66,32 @@ export class DefaultConfigService implements ConfigService { private stateProvider: StateProvider, private authService: AuthService, ) { - const apiUrl$ = this.environmentService.environment$.pipe( - map((environment) => environment.getApiUrl()), - ); const userId$ = this.stateProvider.activeUserId$; const authStatus$ = userId$.pipe( switchMap((userId) => (userId == null ? of(null) : this.authService.authStatusFor$(userId))), ); - this.serverConfig$ = combineLatest([userId$, apiUrl$, authStatus$]).pipe( - switchMap(([userId, apiUrl, authStatus]) => { + this.serverConfig$ = combineLatest([ + userId$, + this.environmentService.environment$, + authStatus$, + ]).pipe( + switchMap(([userId, environment, authStatus]) => { if (userId == null || authStatus !== AuthenticationStatus.Unlocked) { - return this.globalConfigFor$(apiUrl).pipe( - map((config) => [config, null, apiUrl] as const), + return this.globalConfigFor$(environment.getApiUrl()).pipe( + map((config) => [config, null, environment] as const), ); } - return this.userConfigFor$(userId).pipe(map((config) => [config, userId, apiUrl] as const)); + return this.userConfigFor$(userId).pipe( + map((config) => [config, userId, environment] as const), + ); }), tap(async (rec) => { - const [existingConfig, userId, apiUrl] = rec; + const [existingConfig, userId, environment] = rec; // Grab new config if older retrieval interval if (!existingConfig || this.olderThanRetrievalInterval(existingConfig.utcDate)) { - await this.renewConfig(existingConfig, userId, apiUrl); + await this.renewConfig(existingConfig, userId, environment); } }), switchMap(([existingConfig]) => { @@ -149,10 +154,20 @@ export class DefaultConfigService implements ConfigService { private async renewConfig( existingConfig: ServerConfig, userId: UserId, - apiUrl: string, + environment: Environment, ): Promise { try { + // Feature flags often have a big impact on user experience, lets ensure we return some value + // somewhat quickly even though it may not be accurate, we won't cancel the HTTP request + // though so that hopefully it can have finished and hydrated a more accurate value. + const handle = setTimeout(() => { + this.logService.info( + "Self-host environment did not respond in time, emitting previous config.", + ); + this.failedFetchFallbackSubject.next(existingConfig); + }, SLOW_EMISSION_GUARD); const response = await this.configApiService.get(userId); + clearTimeout(handle); const newConfig = new ServerConfig(new ServerConfigData(response)); // Update the environment region @@ -167,7 +182,7 @@ export class DefaultConfigService implements ConfigService { if (userId == null) { // update global state with new pulled config await this.stateProvider.getGlobal(GLOBAL_SERVER_CONFIGURATIONS).update((configs) => { - return { ...configs, [apiUrl]: newConfig }; + return { ...configs, [environment.getApiUrl()]: newConfig }; }); } else { // update state with new pulled config @@ -175,9 +190,7 @@ export class DefaultConfigService implements ConfigService { } } catch (e) { // mutate error to be handled by catchError - this.logService.error( - `Unable to fetch ServerConfig from ${apiUrl}: ${(e as Error)?.message}`, - ); + this.logService.error(`Unable to fetch ServerConfig from ${environment.getApiUrl()}`, e); // Emit the existing config this.failedFetchFallbackSubject.next(existingConfig); }