From 897500dd54151ffefb8c96d4c34ed0692ac2e3af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ivar=20Conradi=20=C3=98sthus?= Date: Mon, 5 Feb 2024 11:24:37 +0100 Subject: [PATCH] fix: version-service should not use process.nextTick (#6124) In the beginning we used process.nextTick() as a trick to load some data initally in the constructor of a service. This is a bad pattern and we should generally avoid any async operations in the constructor. Today we have two alternatives: 1. Defer loading until data is needed (wrap it in async) 2. Use the schdule-service. --- .../instance-stats/instance-stats-service.ts | 2 +- src/lib/routes/admin-api/config.ts | 2 +- src/lib/services/version-service.test.ts | 6 ++-- src/lib/services/version-service.ts | 31 +++++++++---------- 4 files changed, 20 insertions(+), 21 deletions(-) diff --git a/src/lib/features/instance-stats/instance-stats-service.ts b/src/lib/features/instance-stats/instance-stats-service.ts index c8c023d44b..60735c5b93 100644 --- a/src/lib/features/instance-stats/instance-stats-service.ts +++ b/src/lib/features/instance-stats/instance-stats-service.ts @@ -211,7 +211,7 @@ export class InstanceStatsService { * use getStatsSnapshot for low latency, sacrificing data-freshness */ async getStats(): Promise { - const versionInfo = this.versionService.getVersionInfo(); + const versionInfo = await this.versionService.getVersionInfo(); const [ featureToggles, users, diff --git a/src/lib/routes/admin-api/config.ts b/src/lib/routes/admin-api/config.ts index 837451e44c..b166f7948c 100644 --- a/src/lib/routes/admin-api/config.ts +++ b/src/lib/routes/admin-api/config.ts @@ -167,7 +167,7 @@ class ConfigController extends Controller { segmentValuesLimit: this.config.segmentValuesLimit, strategySegmentsLimit: this.config.strategySegmentsLimit, frontendApiOrigins: frontendSettings.frontendApiOrigins, - versionInfo: this.versionService.getVersionInfo(), + versionInfo: await this.versionService.getVersionInfo(), networkViewEnabled: this.config.prometheusApi !== undefined, disablePasswordAuth, maintenanceMode, diff --git a/src/lib/services/version-service.test.ts b/src/lib/services/version-service.test.ts index 28244282fc..57371ba279 100644 --- a/src/lib/services/version-service.test.ts +++ b/src/lib/services/version-service.test.ts @@ -44,7 +44,7 @@ test('yields current versions', async () => { createFakeGetProductionChanges(), ); await service.checkLatestVersion(); - const versionInfo = service.getVersionInfo(); + const versionInfo = await service.getVersionInfo(); expect(scope.isDone()).toEqual(true); expect(versionInfo.current.oss).toBe(version); expect(versionInfo.current.enterprise).toBeFalsy(); @@ -83,7 +83,7 @@ test('supports setting enterprise version as well', async () => { createFakeGetProductionChanges(), ); await service.checkLatestVersion(); - const versionInfo = service.getVersionInfo(); + const versionInfo = await service.getVersionInfo(); expect(scope.isDone()).toEqual(true); expect(versionInfo.current.oss).toBe(version); expect(versionInfo.current.enterprise).toBe(enterpriseVersion); @@ -122,7 +122,7 @@ test('if version check is not enabled should not make any calls', async () => { createFakeGetProductionChanges(), ); await service.checkLatestVersion(); - const versionInfo = service.getVersionInfo(); + const versionInfo = await service.getVersionInfo(); expect(scope.isDone()).toEqual(false); expect(versionInfo.current.oss).toBe(version); expect(versionInfo.current.enterprise).toBe(enterpriseVersion); diff --git a/src/lib/services/version-service.ts b/src/lib/services/version-service.ts index a966490ac6..812b7a2808 100644 --- a/src/lib/services/version-service.ts +++ b/src/lib/services/version-service.ts @@ -178,35 +178,36 @@ export default class VersionService { this.telemetryEnabled = telemetry; this.versionCheckUrl = versionCheck.url; this.isLatest = true; - process.nextTick(() => this.setup()); } - async setup(): Promise { - await this.readInstanceId(); - await this.checkLatestVersion(); - } - - private async readInstanceId(): Promise { + private async readInstanceId(): Promise { try { const { id } = (await this.settingStore.get<{ id: string }>( 'instanceInfo', )) ?? { id: undefined }; - this.instanceId = id; + return id; } catch (err) { this.logger.warn('Could not find instanceInfo', err); } } - getInstanceId() { + async getInstanceId() { + if (!this.instanceId) { + this.instanceId = await this.readInstanceId(); + } return this.instanceId; } async checkLatestVersion(): Promise { + const instanceId = await this.getInstanceId(); + this.logger.debug( + `Checking for newest version for instanceId=${instanceId}`, + ); if (this.enabled) { try { const versionPayload: any = { versions: this.current, - instanceId: this.instanceId, + instanceId: instanceId, }; if (this.telemetryEnabled) { @@ -282,7 +283,7 @@ export default class VersionService { this.userStats(), this.productionChanges(), ]); - const versionInfo = this.getVersionInfo(); + const versionInfo = await this.getVersionInfo(); const customStrategies = await this.strategyStore.getEditableStrategies(); const customStrategiesInUse = @@ -351,15 +352,13 @@ export default class VersionService { return settings?.enabled || false; } - getVersionInfo(): IVersionHolder { + async getVersionInfo(): Promise { + const instanceId = await this.getInstanceId(); return { current: this.current, latest: this.latest || {}, isLatest: this.isLatest, - // @ts-ignore instance id can be undefined but not on the version. What should we do is still unclear. - instanceId: this.instanceId, + instanceId: instanceId || 'unresolved-instance-id', }; } } - -module.exports = VersionService;