From 0e7d3b5ae2cc280079a90b2934310c2dd1b9085c Mon Sep 17 00:00:00 2001 From: Brian Ingles Date: Thu, 11 Jul 2024 16:07:57 -0500 Subject: [PATCH] fix: Disconnect client explicitly (#25) Client wasn't actually getting disconnected when user clicked disconnect button. I have introduced proper disposal logic to dispose all DH services as part of clearing caches. ### Testing #### Test 1 - Start DH server - Connect to server from vscode - Disconnect via the status bar item - Should see "Disconnected from Deephaven server: ..." toast message - Kill server. - Should **not** see a new "Disconnected from Deephaven server..." toast message #### Test 2 - Start DH server - Connect to server from vscode - Kill server - Should see "Disconnected from Deephaven server..." toast message resolves #22 --- src/common/types.d.ts | 4 ++++ src/extension.ts | 10 +++++++--- src/services/CacheService.ts | 30 +++++++++++++++++++++++++----- src/services/DhService.ts | 26 +++++++++++++++----------- src/services/DhServiceRegistry.ts | 5 +++-- src/services/EventDispatcher.ts | 4 +++- src/util/index.ts | 1 + src/util/isDisposable.ts | 17 +++++++++++++++++ 8 files changed, 75 insertions(+), 22 deletions(-) create mode 100644 src/util/isDisposable.ts diff --git a/src/common/types.d.ts b/src/common/types.d.ts index db53c428..a600ee16 100644 --- a/src/common/types.d.ts +++ b/src/common/types.d.ts @@ -4,3 +4,7 @@ export type ConnectionAndSession = { cn: TConnection; session: TSession; }; + +export interface Disposable { + dispose(): Promise; +} diff --git a/src/extension.ts b/src/extension.ts index d2ba06be..22f5e8a4 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -44,20 +44,23 @@ export function activate(context: vscode.ExtensionContext) { outputChannel ); - dhcServiceRegistry.addEventListener('disconnect', () => { + dhcServiceRegistry.addEventListener('disconnect', serverUrl => { + vscode.window.showInformationMessage( + `Disconnected from Deephaven server: ${serverUrl}` + ); clearConnection(); }); /* * Clear connection data */ - function clearConnection() { + async function clearConnection() { selectedConnectionUrl = null; selectedDhService = null; const { text, tooltip } = createConnectTextAndTooltip('disconnected'); connectStatusBarItem.text = text; connectStatusBarItem.tooltip = tooltip; - dhcServiceRegistry.clearCache(); + await dhcServiceRegistry.clearCache(); } /** @@ -85,6 +88,7 @@ export function activate(context: vscode.ExtensionContext) { const connectStatusBarItem = createConnectStatusBarItem(); context.subscriptions.push( + dhcServiceRegistry, outputChannel, runCodeCmd, runSelectionCmd, diff --git a/src/services/CacheService.ts b/src/services/CacheService.ts index 1b92d061..47de24e6 100644 --- a/src/services/CacheService.ts +++ b/src/services/CacheService.ts @@ -1,9 +1,11 @@ +import { Disposable } from '../common'; +import { isDisposable } from '../util'; import { EventDispatcher } from './EventDispatcher'; -export class CacheService< - T, - TEventName extends string -> extends EventDispatcher { +export class CacheService + extends EventDispatcher + implements Disposable +{ constructor( label: string, loader: (key: string | null) => Promise, @@ -35,7 +37,25 @@ export class CacheService< return this.cachedPromises.get(normalizeKey)!; } - public clearCache(): void { + public async clearCache(): Promise { + try { + const allValues = await Promise.all([...this.cachedPromises.values()]); + + // Since the cache is responsible for creating the promises, it is also + // responsible for disposing of them. + allValues.forEach(value => { + if (isDisposable(value)) { + value.dispose(); + } + }); + } catch (err) { + console.error('An error occurred while disposing cached values:', err); + } + this.cachedPromises.clear(); } + + public async dispose(): Promise { + await this.clearCache(); + } } diff --git a/src/services/DhService.ts b/src/services/DhService.ts index 2d9b72a6..faee8637 100644 --- a/src/services/DhService.ts +++ b/src/services/DhService.ts @@ -1,7 +1,7 @@ import * as vscode from 'vscode'; import type { dh as DhcType } from '../dh/dhc-types'; import { hasErrorCode } from '../util/typeUtils'; -import { ConnectionAndSession } from '../common'; +import { ConnectionAndSession, Disposable } from '../common'; import { ExtendedMap, formatTimestamp } from '../util'; import { EventDispatcher } from './EventDispatcher'; @@ -26,10 +26,10 @@ type CommandResultBase = { error: string; }; -export abstract class DhService< - TDH, - TClient -> extends EventDispatcher<'disconnect'> { +export abstract class DhService + extends EventDispatcher<'disconnect'> + implements Disposable +{ constructor( serverUrl: string, panelRegistry: ExtendedMap, @@ -82,13 +82,23 @@ export abstract class DhService< this.cachedCreateSession = null; this.cachedInitApi = null; this.client = null; + + if (this.cn != null) { + this.cn.close(); + this.dispatchEvent('disconnect', this.serverUrl); + } this.cn = null; + this.dh = null; this.session = null; this.subscriptions.forEach(dispose => dispose()); } + public async dispose(): Promise { + this.clearCaches(); + } + public get isInitialized(): boolean { return this.cachedInitApi != null; } @@ -134,12 +144,6 @@ export abstract class DhService< this.subscriptions.push( cn.addEventListener('disconnect', () => { this.clearCaches(); - - vscode.window.showInformationMessage( - `Disconnected from Deephaven server: ${this.serverUrl}` - ); - - this.dispatchEvent('disconnect'); }) ); } diff --git a/src/services/DhServiceRegistry.ts b/src/services/DhServiceRegistry.ts index 033806bd..7b902a5d 100644 --- a/src/services/DhServiceRegistry.ts +++ b/src/services/DhServiceRegistry.ts @@ -25,8 +25,9 @@ export class DhServiceRegistry extends CacheService< outputChannel ); - dhService.addEventListener('disconnect', () => { - this.dispatchEvent('disconnect'); + // Propagate service events as registry events. + dhService.addEventListener('disconnect', event => { + this.dispatchEvent('disconnect', event); }); return dhService; diff --git a/src/services/EventDispatcher.ts b/src/services/EventDispatcher.ts index 74a74956..91180915 100644 --- a/src/services/EventDispatcher.ts +++ b/src/services/EventDispatcher.ts @@ -34,6 +34,8 @@ export class EventDispatcher { * @param event The event to dispatch to all listeners */ dispatchEvent = (eventName: TEventName, event?: TEvent): void => { - this.listeners.get(eventName)?.forEach(listener => listener(event)); + this.listeners.get(eventName)?.forEach(listener => { + listener(event); + }); }; } diff --git a/src/util/index.ts b/src/util/index.ts index 0eb2c6b3..eac63594 100644 --- a/src/util/index.ts +++ b/src/util/index.ts @@ -1,5 +1,6 @@ export * from './downloadUtils'; export * from './ExtendedMap'; +export * from './isDisposable'; export * from './panelUtils'; export * from './polyfillUtils'; export * from './uiUtils'; diff --git a/src/util/isDisposable.ts b/src/util/isDisposable.ts new file mode 100644 index 00000000..2c36cfb1 --- /dev/null +++ b/src/util/isDisposable.ts @@ -0,0 +1,17 @@ +import { Disposable } from '../common'; + +/** + * Typeguard to determine if given object has a `disposable` method. + * @param maybeDisposable The object to check. + * @returns `true` if the object has a `dispose` method, `false` otherwise. + */ +export function isDisposable( + maybeDisposable: unknown +): maybeDisposable is Disposable { + return ( + maybeDisposable != null && + typeof maybeDisposable === 'object' && + 'dispose' in maybeDisposable && + typeof maybeDisposable.dispose === 'function' + ); +}