Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Disposal Logic + Error toast #175

Merged
merged 13 commits into from
Nov 15, 2024
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@
"test:lint": "eslint . --ext ts",
"test:unit": "vitest --reporter=default --reporter=junit --outputFile=./test-reports/vitest.junit.xml",
"test": "npm run test:unit",
"vscode:prepublish": "compile:prod",
"vscode:prepublish": "npm run compile:prod",
"watch:esbuild": "node scripts/esbuild.js --watch",
"watch:tsc": "tsc --build ./tsconfig.json --watch",
"watch": "npm-run-all -p watch:*"
Expand Down
3 changes: 2 additions & 1 deletion src/controllers/ConnectionController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,14 @@ import {
createConnectionQuickPick,
createConnectionQuickPickOptions,
createConnectStatusBarItem,
getConnectionsForConsoleType,
getConsoleType,
getEditorForUri,
isSupportedLanguageId,
Logger,
updateConnectionStatusBarItem,
} from '../util';
import { UnsupportedConsoleTypeError } from '../common';
import { getConnectionsForConsoleType } from '../services';

const logger = new Logger('ConnectionController');

Expand Down Expand Up @@ -153,6 +153,7 @@ export class ConnectionController implements Disposable {
// disconnect from it.
if (err instanceof UnsupportedConsoleTypeError && newConnectionUrl) {
this._serverManager.disconnectFromServer(newConnectionUrl);
this._toaster.error(err.message);
}

throw err;
Expand Down
47 changes: 38 additions & 9 deletions src/controllers/ExtensionController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -117,11 +117,14 @@ export class ExtensionController implements Disposable {
readonly _config: IConfigService;

private _connectionController: ConnectionController | null = null;
private _coreClientCache: URLMap<CoreAuthenticatedClient> | null = null;
private _coreClientCache: URLMap<
CoreAuthenticatedClient & Disposable
> | null = null;
private _coreClientFactory: ICoreClientFactory | null = null;
private _coreJsApiCache: IAsyncCacheService<URL, typeof DhcType> | null =
null;
private _dheClientCache: URLMap<DheAuthenticatedClient> | null = null;
private _dheClientCache: URLMap<DheAuthenticatedClient & Disposable> | null =
null;
private _dheClientFactory: IDheClientFactory | null = null;
private _dheServiceCache: IAsyncCacheService<URL, IDheService> | null = null;
private _panelController: PanelController | null = null;
Expand Down Expand Up @@ -161,6 +164,7 @@ export class ExtensionController implements Disposable {
const codelensProvider = new RunCommandCodeLensProvider();

this._context.subscriptions.push(
codelensProvider,
vscode.languages.registerCodeLensProvider('groovy', codelensProvider),
vscode.languages.registerCodeLensProvider('python', codelensProvider)
);
Expand Down Expand Up @@ -329,22 +333,43 @@ export class ExtensionController implements Disposable {

this._coreClientFactory = async (
url: URL
): Promise<CoreUnauthenticatedClient> => {
): Promise<CoreUnauthenticatedClient & Disposable> => {
assertDefined(this._coreJsApiCache, 'coreJsApiCache');
const dhc = await this._coreJsApiCache.get(url);
return new dhc.CoreClient(url.toString()) as CoreUnauthenticatedClient;

const client = new dhc.CoreClient(
url.toString()
) as CoreUnauthenticatedClient;

// Attach a dispose method so that client caches can dispose of the client
return Object.assign(client, {
dispose: async () => {
client.disconnect();
},
});
};

this._dheClientFactory = async (
url: URL
): Promise<DheUnauthenticatedClient> => {
): Promise<DheUnauthenticatedClient & Disposable> => {
assertDefined(this._dheJsApiCache, 'dheJsApiCache');
const dhe = await this._dheJsApiCache.get(url);
return createDheClient(dhe, getWsUrl(url));

const client = await createDheClient(dhe, getWsUrl(url));

// Attach a dispose method so that client caches can dispose of the client
return Object.assign(client, {
dispose: async () => {
client.disconnect();
},
});
};

this._coreClientCache = new URLMap();
this._dheClientCache = new URLMap();
this._coreClientCache = new URLMap<CoreAuthenticatedClient & Disposable>();
this._context.subscriptions.push(this._coreClientCache);

this._dheClientCache = new URLMap<DheAuthenticatedClient & Disposable>();
this._context.subscriptions.push(this._dheClientCache);

this._panelService = new PanelService();
this._context.subscriptions.push(this._panelService);
Expand Down Expand Up @@ -382,6 +407,7 @@ export class ExtensionController implements Disposable {

this._serverManager.onDidDisconnect(
serverUrl => {
this._panelService?.clearServerData(serverUrl);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes #145

this._outputChannel?.appendLine(
`Disconnected from server: '${serverUrl}'.`
);
Expand Down Expand Up @@ -529,7 +555,10 @@ export class ExtensionController implements Disposable {
this._context.subscriptions.push(
this._serverTreeView,
this._serverConnectionTreeView,
this._serverConnectionPanelTreeView
this._serverConnectionPanelTreeView,
this._serverTreeProvider,
this._serverConnectionTreeProvider,
this._serverConnectionPanelTreeProvider
);
};

Expand Down
3 changes: 2 additions & 1 deletion src/controllers/PipServerController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,9 @@ import {
PIP_SERVER_SUPPORTED_PLATFORMS,
PYTHON_ENV_WAIT,
} from '../common';
import { pollUntilTrue, waitFor } from '../util/promiseUtils';
import { waitFor } from '../util';
import { isDhcServerRunning } from '../dh/dhc';
import { pollUntilTrue } from '../services';

const logger = new Logger('PipServerController');

Expand Down
10 changes: 0 additions & 10 deletions src/dh/dhc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,16 +20,6 @@ export type ConnectionAndSession<TConnection, TSession> = {
session: TSession;
};

// TODO: https://github.com/deephaven/deephaven-core/issues/5911 to address the
// underlying issue of jsapi-types being unaware of `dhinternal`. Once that is
// addressed, this can be removed.
declare global {
// eslint-disable-next-line no-unused-vars
namespace dhinternal.io.deephaven.proto.ticket_pb {
export type TypedTicket = unknown;
}
}

/**
* Download the DH Core jsapi from a running server and return the `dh` object.
* @param serverUrl URL of the DH Core server to download the api from.
Expand Down
7 changes: 6 additions & 1 deletion src/providers/RunCommandCodeLensProvider.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
import * as vscode from 'vscode';
import { ICON_ID } from '../common';
import type { Disposable } from '../types';

/**
* Provides inline editor code lenses for running Deephaven code.
*/
export class RunCommandCodeLensProvider
implements vscode.CodeLensProvider<vscode.CodeLens>
implements vscode.CodeLensProvider<vscode.CodeLens>, Disposable
{
constructor() {
vscode.workspace.onDidChangeConfiguration(() => {
Expand All @@ -22,6 +23,10 @@ export class RunCommandCodeLensProvider
readonly onDidChangeCodeLenses: vscode.Event<void> =
this._onDidChangeCodeLenses.event;

dispose = async (): Promise<void> => {
this._onDidChangeCodeLenses.dispose();
};

provideCodeLenses(
document: vscode.TextDocument,
_token: vscode.CancellationToken
Expand Down
6 changes: 5 additions & 1 deletion src/providers/ServerConnectionPanelTreeProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
getPanelVariableTreeItem,
sortByStringProp,
} from '../util';
import { getFirstSupportedConsoleType } from '../services';

export class ServerConnectionPanelTreeProvider extends TreeDataProviderBase<ServerConnectionPanelNode> {
constructor(serverManager: IServerManager, panelService: IPanelService) {
Expand All @@ -31,7 +32,10 @@ export class ServerConnectionPanelTreeProvider extends TreeDataProviderBase<Serv
return getPanelVariableTreeItem(connectionOrVariable);
}

return getPanelConnectionTreeItem(connectionOrVariable);
return getPanelConnectionTreeItem(
connectionOrVariable,
getFirstSupportedConsoleType
);
};

getChildren = (
Expand Down
8 changes: 6 additions & 2 deletions src/providers/TreeDataProviderBase.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import * as vscode from 'vscode';
import type { IServerManager } from '../types';
import type { Disposable, IServerManager } from '../types';

/**
* Base class for tree view data providers.
*/
export abstract class TreeDataProviderBase<T>
implements vscode.TreeDataProvider<T>
implements vscode.TreeDataProvider<T>, Disposable
{
constructor(serverManager: IServerManager) {
this.serverManager = serverManager;
Expand All @@ -27,6 +27,10 @@ export abstract class TreeDataProviderBase<T>

abstract getChildren(element?: T | undefined): vscode.ProviderResult<T[]>;

dispose = async (): Promise<void> => {
this._onDidChangeTreeData.dispose();
};

refresh(): void {
this._onDidChangeTreeData.fire();
}
Expand Down
15 changes: 8 additions & 7 deletions src/services/DhcService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ export class DhcService implements IDhcService {
try {
const { cn, session } = await this.initSessionPromise;

cn.subscribeToFieldUpdates(changes => {
const fieldUpdateSubscription = cn.subscribeToFieldUpdates(changes => {
this.panelService.updateVariables(
this.serverUrl,
changes as VariableChanges
Expand All @@ -198,15 +198,15 @@ export class DhcService implements IDhcService {
panelVariablesToUpdate
);
});
this.subscriptions.push(fieldUpdateSubscription);

// TODO: Use constant 'disconnect' event name
this.subscriptions.push(
cn.addEventListener('disconnect', () => {
this.clearCaches();
})
);
const disconnectSubscription = cn.addEventListener('disconnect', () => {
this.clearCaches();
});
this.subscriptions.push(disconnectSubscription);

session.onLogMessage(logItem => {
const logMessageSubscription = session.onLogMessage(logItem => {
// TODO: Should this pull log level from config somewhere?
if (logItem.logLevel !== 'INFO') {
const date = new Date(logItem.micros / 1000);
Expand All @@ -217,6 +217,7 @@ export class DhcService implements IDhcService {
);
}
});
this.subscriptions.push(logMessageSubscription);
} catch (err) {}
}

Expand Down
6 changes: 4 additions & 2 deletions src/services/DheService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -256,8 +256,10 @@ export class DheService implements IDheService {
const querySerials = [...this._querySerialSet];

this._querySerialSet.clear();
this._workerInfoMap.clear();

await this._disposeQueries(querySerials);
await Promise.all([
this._workerInfoMap.dispose(),
this._disposeQueries(querySerials),
]);
};
}
47 changes: 0 additions & 47 deletions src/services/EventDispatcher.ts

This file was deleted.

19 changes: 18 additions & 1 deletion src/services/PanelService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,25 @@ export class PanelService implements IPanelService, Disposable {
private readonly _cnPanelMap: URLMap<VariablePanelMap>;
private readonly _cnVariableMap: URLMap<VariableMap>;

/**
* Clear panel data for the given connection url.
* @param url The connection url.
*/
clearServerData = (url: URL): void => {
this._cnPanelMap.delete(url);
this._cnVariableMap.delete(url);
};

/**
* Cleanup resources.
*/
dispose = async (): Promise<void> => {
this._cnPanelMap.clear();
this._onDidUpdate.dispose();

await Promise.all([
this._cnPanelMap.dispose(),
this._cnVariableMap.dispose(),
]);
};

/**
Expand Down
Loading