mirror of
https://github.com/bitwarden/browser.git
synced 2024-11-22 11:45:59 +01:00
[PM-8748] Ensure Reasonable ConfigService Emission (#10452)
* Add Slow Emission Guard * Remove Jest Timer Call * Do Slow Emission Guard For All Environments * Update Comment
This commit is contained in:
parent
c56736e5e2
commit
c18a7cb3e8
@ -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<Error>((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<Environment>(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<ServerConfigResponse>((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;
|
||||
}
|
||||
|
@ -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<ServerConfig>(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<void> {
|
||||
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);
|
||||
}
|
||||
|
Loading…
Reference in New Issue
Block a user