From d08cac81c438e560cc26546f2d14e5f7d6ffa12b Mon Sep 17 00:00:00 2001 From: Sebastian Malton Date: Wed, 19 May 2021 11:27:28 -0400 Subject: [PATCH] Add automatic cleanup on Singleton removal - simplify WindowsManager Signed-off-by: Sebastian Malton --- src/common/utils/singleton.ts | 9 ++- src/main/catalog-sources/kubeconfig-sync.ts | 42 +++++------ src/main/cluster-manager.ts | 74 +++++++++----------- src/main/exit-app.ts | 18 ++--- src/main/helm/__tests__/helm-service.test.ts | 4 +- src/main/index.ts | 9 ++- src/main/window-manager.ts | 37 +++++----- 7 files changed, 88 insertions(+), 105 deletions(-) diff --git a/src/common/utils/singleton.ts b/src/common/utils/singleton.ts index 6b53a19537..7bd6011cb1 100644 --- a/src/common/utils/singleton.ts +++ b/src/common/utils/singleton.ts @@ -19,12 +19,16 @@ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. */ +import { disposer } from "./disposer"; + type StaticThis = { new(...args: R): T }; export class Singleton { private static instances = new WeakMap(); private static creating = ""; + protected disposers = disposer(); + constructor() { if (Singleton.creating.length === 0) { throw new TypeError("A singleton class must be created by createInstance()"); @@ -43,7 +47,7 @@ export class Singleton { * @param args The constructor arguments for the child class * @returns An instance of the child class */ - static createInstance(this: StaticThis, ...args: R): T { + static createInstance(this: StaticThis, ...args: R): T { if (!Singleton.instances.has(this)) { if (Singleton.creating.length > 0) { throw new TypeError("Cannot create a second singleton while creating a first"); @@ -64,7 +68,7 @@ export class Singleton { * Default: `true` * @returns An instance of the child class */ - static getInstance(this: StaticThis, strict = true): T | undefined { + static getInstance(this: StaticThis, strict = true): T | undefined { if (!Singleton.instances.has(this) && strict) { throw new TypeError(`instance of ${this.name} is not created`); } @@ -80,6 +84,7 @@ export class Singleton { * There is *no* way in JS or TS to prevent globals like that. */ static resetInstance() { + Singleton.instances.get(this)?.disposers(); Singleton.instances.delete(this); } } diff --git a/src/main/catalog-sources/kubeconfig-sync.ts b/src/main/catalog-sources/kubeconfig-sync.ts index c4650644da..6fc1f1ff67 100644 --- a/src/main/catalog-sources/kubeconfig-sync.ts +++ b/src/main/catalog-sources/kubeconfig-sync.ts @@ -42,8 +42,6 @@ const logPrefix = "[KUBECONFIG-SYNC]:"; export class KubeconfigSyncManager extends Singleton { protected sources = observable.map, Disposer]>(); protected syncing = false; - protected syncListDisposer?: Disposer; - protected static readonly syncName = "lens:kube-sync"; constructor() { @@ -76,28 +74,26 @@ export class KubeconfigSyncManager extends Singleton { this.startNewSync(filePath); } - this.syncListDisposer = observe(UserStore.getInstance().syncKubeconfigEntries, change => { - switch (change.type) { - case "add": - this.startNewSync(change.name); - break; - case "delete": - this.stopOldSync(change.name); - break; + this.disposers.push( + observe(UserStore.getInstance().syncKubeconfigEntries, change => { + switch (change.type) { + case "add": + this.startNewSync(change.name); + break; + case "delete": + this.stopOldSync(change.name); + break; + } + }), + () => { + for (const filePath of this.sources.keys()) { + this.stopOldSync(filePath); + } + + catalogEntityRegistry.removeSource(KubeconfigSyncManager.syncName); + this.syncing = false; } - }); - } - - @action - stopSync() { - this.syncListDisposer?.(); - - for (const filePath of this.sources.keys()) { - this.stopOldSync(filePath); - } - - catalogEntityRegistry.removeSource(KubeconfigSyncManager.syncName); - this.syncing = false; + ); } @action diff --git a/src/main/cluster-manager.ts b/src/main/cluster-manager.ts index b0eb2e7471..3551fb78b1 100644 --- a/src/main/cluster-manager.ts +++ b/src/main/cluster-manager.ts @@ -27,9 +27,9 @@ import { ClusterStore, getClusterIdFromHost } from "../common/cluster-store"; import type { Cluster } from "./cluster"; import logger from "./logger"; import { apiKubePrefix } from "../common/vars"; -import { Singleton } from "../common/utils"; +import { Singleton, toJS } from "../common/utils"; import { catalogEntityRegistry } from "./catalog"; -import { KubernetesCluster, KubernetesClusterPrometheusMetrics } from "../common/catalog-entities/kubernetes-cluster"; +import { KubernetesCluster } from "../common/catalog-entities/kubernetes-cluster"; export class ClusterManager extends Singleton { private store = ClusterStore.getInstance(); @@ -37,35 +37,37 @@ export class ClusterManager extends Singleton { constructor() { super(); makeObservable(this); - this.bindEvents(); - } - private bindEvents() { - // reacting to every cluster's state change and total amount of items - reaction( - () => this.store.clustersList.map(c => c.getState()), - () => this.updateCatalog(this.store.clustersList), - { fireImmediately: true, } - ); + this.disposers.push( + reaction( + () => toJS(this.store.clustersList), + clusters => this.updateCatalog(clusters), + { fireImmediately: true }, + ), + reaction( + () => catalogEntityRegistry.getItemsForApiKind("entity.k8slens.dev/v1alpha1", "KubernetesCluster"), + entities => this.syncClustersFromCatalog(entities) + ), + // auto-stop removed clusters + autorun(() => { + const removedClusters = Array.from(this.store.removedClusters.values()); - reaction(() => catalogEntityRegistry.getItemsForApiKind("entity.k8slens.dev/v1alpha1", "KubernetesCluster"), (entities) => { - this.syncClustersFromCatalog(entities); - }); + if (removedClusters.length > 0) { + const meta = removedClusters.map(cluster => cluster.getMeta()); - // auto-stop removed clusters - autorun(() => { - const removedClusters = Array.from(this.store.removedClusters.values()); - - if (removedClusters.length > 0) { - const meta = removedClusters.map(cluster => cluster.getMeta()); - - logger.info(`[CLUSTER-MANAGER]: removing clusters`, meta); - removedClusters.forEach(cluster => cluster.disconnect()); - this.store.removedClusters.clear(); + logger.info(`[CLUSTER-MANAGER]: removing clusters`, meta); + removedClusters.forEach(cluster => cluster.disconnect()); + this.store.removedClusters.clear(); + } + }, { + delay: 250 + }), + () => { + for (const cluster of this.store.clusters.values()) { + cluster.disconnect(); + } } - }, { - delay: 250 - }); + ); ipcMain.on("network:offline", this.onNetworkOffline); ipcMain.on("network:online", this.onNetworkOnline); @@ -77,7 +79,7 @@ export class ClusterManager extends Singleton { const index = catalogEntityRegistry.items.findIndex((entity) => entity.metadata.uid === cluster.id); if (index !== -1) { - const entity = catalogEntityRegistry.items[index] as KubernetesCluster; + const entity = catalogEntityRegistry.items[index]; entity.status.phase = cluster.disconnected ? "disconnected" : "connected"; entity.status.active = !cluster.disconnected; @@ -86,14 +88,12 @@ export class ClusterManager extends Singleton { entity.metadata.name = cluster.preferences.clusterName; } - entity.spec.metrics ||= { source: "local" }; + entity.spec.metrics ??= { source: "local" }; if (entity.spec.metrics.source === "local") { - const prometheus: KubernetesClusterPrometheusMetrics = entity.spec?.metrics?.prometheus || {}; - - prometheus.type = cluster.preferences.prometheusProvider?.type; - prometheus.address = cluster.preferences.prometheus; - entity.spec.metrics.prometheus = prometheus; + entity.spec.metrics.prometheus ??= {}; + entity.spec.metrics.prometheus.type ??= cluster.preferences.prometheusProvider?.type; + entity.spec.metrics.prometheus.address = cluster.preferences.prometheus; } catalogEntityRegistry.items.splice(index, 1, entity); @@ -146,12 +146,6 @@ export class ClusterManager extends Singleton { }); }; - stop() { - this.store.clusters.forEach((cluster: Cluster) => { - cluster.disconnect(); - }); - } - getClusterForRequest(req: http.IncomingMessage): Cluster { let cluster: Cluster = null; diff --git a/src/main/exit-app.ts b/src/main/exit-app.ts index 2f9e3b00c7..a8bcf76525 100644 --- a/src/main/exit-app.ts +++ b/src/main/exit-app.ts @@ -26,19 +26,11 @@ import { ClusterManager } from "./cluster-manager"; import logger from "./logger"; export function exitApp() { - console.log("before windowManager"); - const windowManager = WindowManager.getInstance(false); - - console.log("before clusterManager"); - const clusterManager = ClusterManager.getInstance(false); - - console.log("after clusterManager"); - appEventBus.emit({ name: "service", action: "close" }); - windowManager?.hide(); - clusterManager?.stop(); + + WindowManager.resetInstance(); + ClusterManager.resetInstance(); logger.info("SERVICE:QUIT"); - setTimeout(() => { - app.exit(); - }, 1000); + + setTimeout(() => app.exit(), 1000); } diff --git a/src/main/helm/__tests__/helm-service.test.ts b/src/main/helm/__tests__/helm-service.test.ts index 6ac169d5d0..ffb5410d43 100644 --- a/src/main/helm/__tests__/helm-service.test.ts +++ b/src/main/helm/__tests__/helm-service.test.ts @@ -40,7 +40,7 @@ describe("Helm Service tests", () => { { name: "experiment", url: "experimenturl" }, ]; }), - }); + } as any); const charts = await helmService.listCharts(); @@ -91,7 +91,7 @@ describe("Helm Service tests", () => { { name: "bitnami", url: "bitnamiurl" }, ]; }), - }); + } as any); const charts = await helmService.listCharts(); diff --git a/src/main/index.ts b/src/main/index.ts index bf8d4943e3..15330e98f5 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -148,7 +148,6 @@ app.on("ready", async () => { const lensProxy = LensProxy.createInstance(handleWsUpgrade); ClusterManager.createInstance(); - KubeconfigSyncManager.createInstance(); try { logger.info("🔌 Starting LensProxy"); @@ -194,7 +193,7 @@ app.on("ready", async () => { ipcMain.on(IpcRendererNavigationEvents.LOADED, () => { cleanup.push(pushCatalogToRenderer(catalogEntityRegistry)); - KubeconfigSyncManager.getInstance().startSync(); + KubeconfigSyncManager.createInstance().startSync(); startUpdateChecking(); LensProtocolRouterMain.getInstance().rendererLoaded = true; }); @@ -252,9 +251,9 @@ app.on("will-quit", (event) => { // Quit app on Cmd+Q (MacOS) logger.info("APP:QUIT"); appEventBus.emit({name: "app", action: "close"}); - ClusterManager.getInstance(false)?.stop(); // close cluster connections - KubeconfigSyncManager.getInstance(false)?.stopSync(); - cleanup(); + WindowManager.resetInstance(); + ClusterManager.resetInstance(); + KubeconfigSyncManager.resetInstance(); if (blockQuit) { event.preventDefault(); // prevent app's default shutdown (e.g. required for telemetry, etc.) diff --git a/src/main/window-manager.ts b/src/main/window-manager.ts index 4e5588e47a..d11026cec7 100644 --- a/src/main/window-manager.ts +++ b/src/main/window-manager.ts @@ -34,11 +34,14 @@ import logger from "./logger"; import { productName } from "../common/vars"; import { LensProxy } from "./proxy/lens-proxy"; +function isHideable(window: BrowserWindow | null): boolean { + return Boolean(window && !window.isDestroyed()); +} + export class WindowManager extends Singleton { - protected mainWindow: BrowserWindow; - protected splashWindow: BrowserWindow; + protected mainWindow: BrowserWindow | null = null; + protected splashWindow: BrowserWindow | null = null; protected windowState: windowStateKeeper.State; - protected disposers: Record = {}; @observable activeClusterId: ClusterId; @@ -46,8 +49,9 @@ export class WindowManager extends Singleton { super(); makeObservable(this); this.bindEvents(); - this.initMenu(); - this.initTray(); + this.disposers.push(initMenu(this)); + this.disposers.push(initTray(this)); + this.disposers.push(() => this.destroy()); } get mainUrl() { @@ -131,14 +135,6 @@ export class WindowManager extends Singleton { } } - protected async initMenu() { - this.disposers.menuAutoUpdater = initMenu(this); - } - - protected initTray() { - this.disposers.trayAutoUpdater = initTray(this); - } - protected bindEvents() { // track visible cluster from ui subscribeToBroadcast(IpcRendererNavigationEvents.CLUSTER_VIEW_CURRENT_ID, (event, clusterId: ClusterId) => { @@ -206,18 +202,19 @@ export class WindowManager extends Singleton { } hide() { - if (this.mainWindow && !this.mainWindow.isDestroyed()) this.mainWindow.hide(); - if (this.splashWindow && !this.splashWindow.isDestroyed()) this.splashWindow.hide(); + if (isHideable(this.mainWindow)) { + this.mainWindow.hide(); + } + + if (isHideable(this.splashWindow)) { + this.splashWindow.hide(); + } } - destroy() { + private destroy() { this.mainWindow.destroy(); this.splashWindow.destroy(); this.mainWindow = null; this.splashWindow = null; - Object.entries(this.disposers).forEach(([name, dispose]) => { - dispose(); - delete this.disposers[name]; - }); } }