From ebdb4d04e4b2d9ea04ac9fd7459b9e7c0410f510 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Mon, 4 Oct 2021 15:28:07 -0700 Subject: [PATCH] Ensure database storage extension uses to track all storages does not grow unnecessarily (#17598) * Ensure central storage to track keys for other storage never contain duplicate entries * News entry * Add tests * Add more test cases * Fix storage for function --- news/2 Fixes/17488.md | 1 + src/client/common/persistentState.ts | 87 ++++++++++++++------ src/client/common/utils/decorators.ts | 8 +- src/test/common/persistentState.unit.test.ts | 46 ++++++++++- 4 files changed, 113 insertions(+), 29 deletions(-) create mode 100644 news/2 Fixes/17488.md diff --git a/news/2 Fixes/17488.md b/news/2 Fixes/17488.md new file mode 100644 index 000000000000..a80def2f42f0 --- /dev/null +++ b/news/2 Fixes/17488.md @@ -0,0 +1 @@ +Ensure database storage extension uses to track all storages does not grow unnecessarily. diff --git a/src/client/common/persistentState.ts b/src/client/common/persistentState.ts index 41d74bf30ba5..c89b30aa805b 100644 --- a/src/client/common/persistentState.ts +++ b/src/client/common/persistentState.ts @@ -8,6 +8,7 @@ import { Memento } from 'vscode'; import { IExtensionSingleActivationService } from '../activation/types'; import { ICommandManager } from './application/types'; import { Commands } from './constants'; +import { traceError, traceVerbose } from './logger'; import { GLOBAL_MEMENTO, IExtensionContext, @@ -16,6 +17,7 @@ import { IPersistentStateFactory, WORKSPACE_MEMENTO, } from './types'; +import { cache } from './utils/decorators'; export class PersistentState implements IPersistentState { constructor( @@ -39,30 +41,39 @@ export class PersistentState implements IPersistentState { } public async updateValue(newValue: T): Promise { - if (this.expiryDurationMs) { - await this.storage.update(this.key, { data: newValue, expiry: Date.now() + this.expiryDurationMs }); - } else { - await this.storage.update(this.key, newValue); + try { + if (this.expiryDurationMs) { + await this.storage.update(this.key, { data: newValue, expiry: Date.now() + this.expiryDurationMs }); + } else { + await this.storage.update(this.key, newValue); + } + } catch (ex) { + traceError('Error while updating storage for key:', this.key, ex); } } } -const GLOBAL_PERSISTENT_KEYS = 'PYTHON_EXTENSION_GLOBAL_STORAGE_KEYS'; -const WORKSPACE_PERSISTENT_KEYS = 'PYTHON_EXTENSION_WORKSPACE_STORAGE_KEYS'; -type keysStorage = { key: string; defaultValue: unknown }; +const GLOBAL_PERSISTENT_KEYS_DEPRECATED = 'PYTHON_EXTENSION_GLOBAL_STORAGE_KEYS'; +const WORKSPACE_PERSISTENT_KEYS_DEPRECATED = 'PYTHON_EXTENSION_WORKSPACE_STORAGE_KEYS'; + +const GLOBAL_PERSISTENT_KEYS = 'PYTHON_GLOBAL_STORAGE_KEYS'; +const WORKSPACE_PERSISTENT_KEYS = 'PYTHON_WORKSPACE_STORAGE_KEYS'; +type KeysStorageType = 'global' | 'workspace'; +type KeysStorage = { key: string; defaultValue: unknown }; @injectable() export class PersistentStateFactory implements IPersistentStateFactory, IExtensionSingleActivationService { - private readonly globalKeysStorage = new PersistentState( + public readonly _globalKeysStorage = new PersistentState( this.globalState, GLOBAL_PERSISTENT_KEYS, [], ); - private readonly workspaceKeysStorage = new PersistentState( + public readonly _workspaceKeysStorage = new PersistentState( this.workspaceState, WORKSPACE_PERSISTENT_KEYS, [], ); + private cleanedOnce = false; constructor( @inject(IMemento) @named(GLOBAL_MEMENTO) private globalState: Memento, @inject(IMemento) @named(WORKSPACE_MEMENTO) private workspaceState: Memento, @@ -71,6 +82,19 @@ export class PersistentStateFactory implements IPersistentStateFactory, IExtensi public async activate(): Promise { this.cmdManager.registerCommand(Commands.ClearStorage, this.cleanAllPersistentStates.bind(this)); + const globalKeysStorageDeprecated = this.createGlobalPersistentState(GLOBAL_PERSISTENT_KEYS_DEPRECATED, []); + const workspaceKeysStorageDeprecated = this.createWorkspacePersistentState( + WORKSPACE_PERSISTENT_KEYS_DEPRECATED, + [], + ); + // Old storages have grown to be unusually large due to https://github.com/microsoft/vscode-python/issues/17488, + // so reset them. This line can be removed after a while. + if (globalKeysStorageDeprecated.value.length > 0) { + globalKeysStorageDeprecated.updateValue([]).ignoreErrors(); + } + if (workspaceKeysStorageDeprecated.value.length > 0) { + workspaceKeysStorageDeprecated.updateValue([]).ignoreErrors(); + } } public createGlobalPersistentState( @@ -78,9 +102,7 @@ export class PersistentStateFactory implements IPersistentStateFactory, IExtensi defaultValue?: T, expiryDurationMs?: number, ): IPersistentState { - if (!this.globalKeysStorage.value.includes({ key, defaultValue })) { - this.globalKeysStorage.updateValue([{ key, defaultValue }, ...this.globalKeysStorage.value]).ignoreErrors(); - } + this.addKeyToStorage('global', key, defaultValue).ignoreErrors(); return new PersistentState(this.globalState, key, defaultValue, expiryDurationMs); } @@ -89,29 +111,44 @@ export class PersistentStateFactory implements IPersistentStateFactory, IExtensi defaultValue?: T, expiryDurationMs?: number, ): IPersistentState { - if (!this.workspaceKeysStorage.value.includes({ key, defaultValue })) { - this.workspaceKeysStorage - .updateValue([{ key, defaultValue }, ...this.workspaceKeysStorage.value]) - .ignoreErrors(); - } + this.addKeyToStorage('workspace', key, defaultValue).ignoreErrors(); return new PersistentState(this.workspaceState, key, defaultValue, expiryDurationMs); } + /** + * Note we use a decorator to cache the promise returned by this method, so it's only called once. + * It is only cached for the particular arguments passed, so the argument type is simplified here. + */ + @cache(-1, true) + private async addKeyToStorage(keyStorageType: KeysStorageType, key: string, defaultValue?: T) { + const storage = keyStorageType === 'global' ? this._globalKeysStorage : this._workspaceKeysStorage; + const found = storage.value.find((value) => value.key === key && value.defaultValue === defaultValue); + if (!found) { + await storage.updateValue([{ key, defaultValue }, ...storage.value]); + } + } + private async cleanAllPersistentStates(): Promise { + if (this.cleanedOnce) { + traceError('Storage can only be cleaned once per session, reload window.'); + return; + } await Promise.all( - this.globalKeysStorage.value.map(async (keyContent) => { + this._globalKeysStorage.value.map(async (keyContent) => { const storage = this.createGlobalPersistentState(keyContent.key); await storage.updateValue(keyContent.defaultValue); }), ); await Promise.all( - this.workspaceKeysStorage.value.map(async (keyContent) => { + this._workspaceKeysStorage.value.map(async (keyContent) => { const storage = this.createWorkspacePersistentState(keyContent.key); await storage.updateValue(keyContent.defaultValue); }), ); - await this.globalKeysStorage.updateValue([]); - await this.workspaceKeysStorage.updateValue([]); + await this._globalKeysStorage.updateValue([]); + await this._workspaceKeysStorage.updateValue([]); + this.cleanedOnce = true; + traceVerbose('Finished clearing storage.'); } } @@ -128,9 +165,11 @@ interface IPersistentStorage { * Build a global storage object for the given key. */ export function getGlobalStorage(context: IExtensionContext, key: string, defaultValue?: T): IPersistentStorage { - const globalKeysStorage = new PersistentState(context.globalState, GLOBAL_PERSISTENT_KEYS, []); - if (!globalKeysStorage.value.includes({ key, defaultValue: undefined })) { - globalKeysStorage.updateValue([{ key, defaultValue: undefined }, ...globalKeysStorage.value]).ignoreErrors(); + const globalKeysStorage = new PersistentState(context.globalState, GLOBAL_PERSISTENT_KEYS, []); + const found = globalKeysStorage.value.find((value) => value.key === key && value.defaultValue === defaultValue); + if (!found) { + const newValue = [{ key, defaultValue }, ...globalKeysStorage.value]; + globalKeysStorage.updateValue(newValue).ignoreErrors(); } const raw = new PersistentState(context.globalState, key, defaultValue); return { diff --git a/src/client/common/utils/decorators.ts b/src/client/common/utils/decorators.ts index c5125a9b0c34..c024e4e9d048 100644 --- a/src/client/common/utils/decorators.ts +++ b/src/client/common/utils/decorators.ts @@ -153,7 +153,13 @@ export function cache(expiryDurationMs: number, cachePromise = false, expiryDura if (isTestExecution()) { return originalMethod.apply(this, args) as Promise; } - const key = getCacheKeyFromFunctionArgs(keyPrefix, args); + let key: string; + try { + key = getCacheKeyFromFunctionArgs(keyPrefix, args); + } catch (ex) { + traceError('Error while creating key for keyPrefix:', keyPrefix, ex); + return originalMethod.apply(this, args) as Promise; + } const cachedItem = cacheStoreForMethods.get(key); if (cachedItem && (cachedItem.expiry > Date.now() || expiryDurationMs === -1)) { traceVerbose(`Cached data exists ${key}`); diff --git a/src/test/common/persistentState.unit.test.ts b/src/test/common/persistentState.unit.test.ts index a4cb10d19a4d..5c4f6fc4770d 100644 --- a/src/test/common/persistentState.unit.test.ts +++ b/src/test/common/persistentState.unit.test.ts @@ -3,19 +3,19 @@ 'use strict'; -import { expect } from 'chai'; +import { assert, expect } from 'chai'; import * as TypeMoq from 'typemoq'; import { Memento } from 'vscode'; -import { IExtensionSingleActivationService } from '../../client/activation/types'; import { ICommandManager } from '../../client/common/application/types'; import { Commands } from '../../client/common/constants'; import { PersistentStateFactory } from '../../client/common/persistentState'; -import { IDisposable, IPersistentStateFactory } from '../../client/common/types'; +import { IDisposable } from '../../client/common/types'; +import { sleep } from '../core'; import { MockMemento } from '../mocks/mementos'; suite('Persistent State', () => { let cmdManager: TypeMoq.IMock; - let persistentStateFactory: IPersistentStateFactory & IExtensionSingleActivationService; + let persistentStateFactory: PersistentStateFactory; let workspaceMemento: Memento; let globalMemento: Memento; setup(() => { @@ -87,4 +87,42 @@ suite('Persistent State', () => { expect(workspaceKey1State.value).to.equal(undefined); expect(workspaceKey2State.value).to.equal('defaultKey2Value'); }); + + test('Ensure internal global storage extension uses to track other storages does not contain duplicate entries', async () => { + persistentStateFactory.createGlobalPersistentState('key1'); + await sleep(1); + persistentStateFactory.createGlobalPersistentState('key2', 'defaultValue1'); + await sleep(1); + persistentStateFactory.createGlobalPersistentState('key2', 'defaultValue1'); + await sleep(1); + persistentStateFactory.createGlobalPersistentState('key1'); + await sleep(1); + const { value } = persistentStateFactory._globalKeysStorage; + assert.deepEqual( + value.sort((k1, k2) => k1.key.localeCompare(k2.key)), + [ + { key: 'key1', defaultValue: undefined }, + { key: 'key2', defaultValue: 'defaultValue1' }, + ].sort((k1, k2) => k1.key.localeCompare(k2.key)), + ); + }); + + test('Ensure internal workspace storage extension uses to track other storages does not contain duplicate entries', async () => { + persistentStateFactory.createWorkspacePersistentState('key2', 'defaultValue1'); + await sleep(1); + persistentStateFactory.createWorkspacePersistentState('key1'); + await sleep(1); + persistentStateFactory.createWorkspacePersistentState('key2', 'defaultValue1'); + await sleep(1); + persistentStateFactory.createWorkspacePersistentState('key1'); + await sleep(1); + const { value } = persistentStateFactory._workspaceKeysStorage; + assert.deepEqual( + value.sort((k1, k2) => k1.key.localeCompare(k2.key)), + [ + { key: 'key1', defaultValue: undefined }, + { key: 'key2', defaultValue: 'defaultValue1' }, + ].sort((k1, k2) => k1.key.localeCompare(k2.key)), + ); + }); });