From 86cde4c511c8fffc926e3fc9e150878a253ef977 Mon Sep 17 00:00:00 2001 From: Charles Gagnon Date: Mon, 29 Jul 2019 11:00:11 -0700 Subject: [PATCH] Fix some connection listener leaks (#6357) * Fix some connection listener leaks * More descriptive name and update summary * Dispose some more connections and fix a few spelling errors --- .../common/capabilitiesService.ts | 2 +- .../common/capabilitiesServiceImpl.ts | 6 +++--- .../common/connectionManagementService.ts | 5 ++++- .../common/connectionProfileGroup.ts | 11 ++++++---- .../common/providerConnectionInfo.ts | 17 ++++++++++++--- .../objectExplorer/browser/treeUpdateUtils.ts | 7 ++++++- .../browser/connectionController.ts | 17 ++++++++------- .../browser/connectionDialogService.ts | 21 ++++++++++++++----- .../browser/connectionDialogWidget.ts | 8 +++++-- 9 files changed, 66 insertions(+), 28 deletions(-) diff --git a/src/sql/platform/capabilities/common/capabilitiesService.ts b/src/sql/platform/capabilities/common/capabilitiesService.ts index 577d902161..82043919f2 100644 --- a/src/sql/platform/capabilities/common/capabilitiesService.ts +++ b/src/sql/platform/capabilities/common/capabilitiesService.ts @@ -55,7 +55,7 @@ export interface ICapabilitiesService { isFeatureAvailable(action: IAction, connectionManagementInfo: ConnectionManagementInfo): boolean; /** - * When a new capabilities is registered, it emits the provider name, be to use to get the new capabilities + * When new capabilities are registered, it emits the @see ProviderFeatures, which can be used to get the new capabilities */ readonly onCapabilitiesRegistered: Event; diff --git a/src/sql/platform/capabilities/common/capabilitiesServiceImpl.ts b/src/sql/platform/capabilities/common/capabilitiesServiceImpl.ts index 2c87134b3a..b7a9912792 100644 --- a/src/sql/platform/capabilities/common/capabilitiesServiceImpl.ts +++ b/src/sql/platform/capabilities/common/capabilitiesServiceImpl.ts @@ -34,7 +34,7 @@ interface CapabilitiesMomento { /** * Capabilities service implementation class. This class provides the ability - * to discover the DMP capabilties that a DMP provider offers. + * to discover the DMP capabilities that a DMP provider offers. */ export class CapabilitiesService extends Disposable implements ICapabilitiesService { _serviceBrand: any; @@ -50,7 +50,7 @@ export class CapabilitiesService extends Disposable implements ICapabilitiesServ constructor( @IStorageService private _storageService: IStorageService, @IExtensionService extensionService: IExtensionService, - @IExtensionManagementService extentionManagementService: IExtensionManagementService + @IExtensionManagementService extensionManagementService: IExtensionManagementService ) { super(); @@ -78,7 +78,7 @@ export class CapabilitiesService extends Disposable implements ICapabilitiesServ _storageService.onWillSaveState(() => this.shutdown()); - this._register(extentionManagementService.onDidUninstallExtension(({ identifier }) => { + this._register(extensionManagementService.onDidUninstallExtension(({ identifier }) => { const connectionProvider = 'connectionProvider'; extensionService.getExtensions().then(i => { let extension = i.find(c => c.identifier.value.toLowerCase() === identifier.id.toLowerCase()); diff --git a/src/sql/platform/connection/common/connectionManagementService.ts b/src/sql/platform/connection/common/connectionManagementService.ts index 862efcb141..b0072d4e9c 100644 --- a/src/sql/platform/connection/common/connectionManagementService.ts +++ b/src/sql/platform/connection/common/connectionManagementService.ts @@ -691,7 +691,10 @@ export class ConnectionManagementService extends Disposable implements IConnecti } public hasRegisteredServers(): boolean { - return this.doHasRegisteredServers(this.getConnectionGroups()); + const groups: ConnectionProfileGroup[] = this.getConnectionGroups(); + const hasRegisteredServers: boolean = this.doHasRegisteredServers(groups); + groups.forEach(cpg => cpg.dispose()); + return hasRegisteredServers; } private doHasRegisteredServers(root: ConnectionProfileGroup[]): boolean { diff --git a/src/sql/platform/connection/common/connectionProfileGroup.ts b/src/sql/platform/connection/common/connectionProfileGroup.ts index 26d8162638..9af73a0e5b 100644 --- a/src/sql/platform/connection/common/connectionProfileGroup.ts +++ b/src/sql/platform/connection/common/connectionProfileGroup.ts @@ -4,6 +4,7 @@ *--------------------------------------------------------------------------------------------*/ import { ConnectionProfile } from 'sql/platform/connection/common/connectionProfile'; +import { Disposable } from 'vs/base/common/lifecycle'; export interface IConnectionProfileGroup { id: string; @@ -13,7 +14,7 @@ export interface IConnectionProfileGroup { description: string; } -export class ConnectionProfileGroup implements IConnectionProfileGroup { +export class ConnectionProfileGroup extends Disposable implements IConnectionProfileGroup { public children: ConnectionProfileGroup[]; public connections: ConnectionProfile[]; @@ -26,6 +27,7 @@ export class ConnectionProfileGroup implements IConnectionProfileGroup { public color: string, public description: string ) { + super(); this.parentId = parent ? parent.id : undefined; if (this.name === ConnectionProfileGroup.RootGroupName) { this.name = ''; @@ -100,9 +102,8 @@ export class ConnectionProfileGroup implements IConnectionProfileGroup { } } - public getChildren(): any { - let allChildren = []; - + public getChildren(): (ConnectionProfile | ConnectionProfileGroup)[] { + let allChildren: (ConnectionProfile | ConnectionProfileGroup)[] = []; if (this.connections) { this.connections.forEach((conn) => { allChildren.push(conn); @@ -131,6 +132,7 @@ export class ConnectionProfileGroup implements IConnectionProfileGroup { connections.forEach((conn) => { this.connections = this.connections.filter((curConn) => { return curConn.id !== conn.id; }); conn.parent = this; + this._register(conn); this.connections.push(conn); }); @@ -143,6 +145,7 @@ export class ConnectionProfileGroup implements IConnectionProfileGroup { groups.forEach((group) => { this.children = this.children.filter((grp) => { return group.id !== grp.id; }); group.parent = this; + this._register(group); this.children.push(group); }); } diff --git a/src/sql/platform/connection/common/providerConnectionInfo.ts b/src/sql/platform/connection/common/providerConnectionInfo.ts index 67df5770a1..c992ad1536 100644 --- a/src/sql/platform/connection/common/providerConnectionInfo.ts +++ b/src/sql/platform/connection/common/providerConnectionInfo.ts @@ -3,7 +3,7 @@ * Licensed under the Source EULA. See License.txt in the project root for license information. *--------------------------------------------------------------------------------------------*/ -import { Disposable } from 'vs/base/common/lifecycle'; +import { Disposable, dispose, IDisposable } from 'vs/base/common/lifecycle'; import { isString } from 'vs/base/common/types'; import * as azdata from 'azdata'; @@ -19,6 +19,7 @@ export class ProviderConnectionInfo extends Disposable implements azdata.Connect options: { [name: string]: any } = {}; private _providerName: string; + private _onCapabilitiesRegisteredDisposable: IDisposable; protected _serverCapabilities: ConnectionProviderProperties; private static readonly SqlAuthentication = 'SqlLogin'; public static readonly ProviderPropertyName = 'providerName'; @@ -74,14 +75,24 @@ export class ProviderConnectionInfo extends Disposable implements azdata.Connect if (capabilities) { this._serverCapabilities = capabilities.connection; } - this._register(this.capabilitiesService.onCapabilitiesRegistered(e => { + if (this._onCapabilitiesRegisteredDisposable) { + dispose(this._onCapabilitiesRegisteredDisposable); + } + this._onCapabilitiesRegisteredDisposable = this.capabilitiesService.onCapabilitiesRegistered(e => { if (e.connection.providerId === this.providerName) { this._serverCapabilities = e.connection; } - })); + }); } } + public dispose(): void { + if (this._onCapabilitiesRegisteredDisposable) { + dispose(this._onCapabilitiesRegisteredDisposable); + } + super.dispose(); + } + public clone(): ProviderConnectionInfo { let instance = new ProviderConnectionInfo(this.capabilitiesService, this.providerName); instance.options = Object.assign({}, this.options); diff --git a/src/sql/workbench/parts/objectExplorer/browser/treeUpdateUtils.ts b/src/sql/workbench/parts/objectExplorer/browser/treeUpdateUtils.ts index 38efca6152..82fbf4a4b2 100644 --- a/src/sql/workbench/parts/objectExplorer/browser/treeUpdateUtils.ts +++ b/src/sql/workbench/parts/objectExplorer/browser/treeUpdateUtils.ts @@ -13,6 +13,7 @@ import { NodeType } from 'sql/workbench/parts/objectExplorer/common/nodeType'; import { TreeNode } from 'sql/workbench/parts/objectExplorer/common/treeNode'; import * as errors from 'vs/base/common/errors'; import { IConnectionProfile } from 'sql/platform/connection/common/interfaces'; +import { Disposable } from 'vs/base/common/lifecycle'; export interface IExpandableTree extends ITree { // {{SQL CARBON EDIT }} - add back deleted VS Code tree methods @@ -72,8 +73,11 @@ export class TreeUpdateUtils { } else if (viewKey === 'saved') { treeInput = TreeUpdateUtils.getTreeInput(connectionManagementService, providers); } - + const previousTreeInput: any = tree.getInput(); return tree.setInput(treeInput).then(() => { + if (previousTreeInput instanceof Disposable) { + previousTreeInput.dispose(); + } // Make sure to expand all folders that where expanded in the previous session if (targetsToExpand) { tree.expandAll(targetsToExpand); @@ -135,6 +139,7 @@ export class TreeUpdateUtils { if (groups && groups.length > 0) { let treeInput = groups[0]; treeInput.name = 'root'; + groups.forEach(cpg => cpg.dispose()); return treeInput; } // Should never get to this case. diff --git a/src/sql/workbench/services/connection/browser/connectionController.ts b/src/sql/workbench/services/connection/browser/connectionController.ts index 21bfe6167a..1b1ecad342 100644 --- a/src/sql/workbench/services/connection/browser/connectionController.ts +++ b/src/sql/workbench/services/connection/browser/connectionController.ts @@ -117,22 +117,22 @@ export class ConnectionController implements IConnectionComponentController { this._connectionWidget.createConnectionWidget(container); } - private getServerGroupHelper(group: ConnectionProfileGroup, groupNames: IConnectionProfileGroup[]): void { + private flattenGroups(group: ConnectionProfileGroup, allGroups: IConnectionProfileGroup[]): void { if (group) { if (group.fullName !== '') { - groupNames.push(group); + allGroups.push(group); } if (group.hasChildren()) { - group.children.forEach((child) => this.getServerGroupHelper(child, groupNames)); + group.children.forEach((child) => this.flattenGroups(child, allGroups)); } } } private getAllServerGroups(providers?: string[]): IConnectionProfileGroup[] { let connectionGroupRoot = this._connectionManagementService.getConnectionGroups(providers); - let connectionGroupNames: IConnectionProfileGroup[] = []; + let allGroups: IConnectionProfileGroup[] = []; if (connectionGroupRoot && connectionGroupRoot.length > 0) { - this.getServerGroupHelper(connectionGroupRoot[0], connectionGroupNames); + this.flattenGroups(connectionGroupRoot[0], allGroups); } let defaultGroupId: string; if (connectionGroupRoot && connectionGroupRoot.length > 0 && ConnectionProfileGroup.isRoot(connectionGroupRoot[0].name)) { @@ -140,9 +140,10 @@ export class ConnectionController implements IConnectionComponentController { } else { defaultGroupId = Utils.defaultGroupId; } - connectionGroupNames.push(Object.assign({}, this._connectionWidget.DefaultServerGroup, { id: defaultGroupId })); - connectionGroupNames.push(this._connectionWidget.NoneServerGroup); - return connectionGroupNames; + allGroups.push(Object.assign({}, this._connectionWidget.DefaultServerGroup, { id: defaultGroupId })); + allGroups.push(this._connectionWidget.NoneServerGroup); + connectionGroupRoot.forEach(cpg => cpg.dispose()); + return allGroups; } public initDialog(providers: string[], connectionInfo: IConnectionProfile): void { diff --git a/src/sql/workbench/services/connection/browser/connectionDialogService.ts b/src/sql/workbench/services/connection/browser/connectionDialogService.ts index ecde583e42..37613bf15c 100644 --- a/src/sql/workbench/services/connection/browser/connectionDialogService.ts +++ b/src/sql/workbench/services/connection/browser/connectionDialogService.ts @@ -316,8 +316,11 @@ export class ConnectionDialogService implements IConnectionDialogService { } } this._model.providerName = this._currentProviderType; - + const previousModel = this._model; this._model = new ConnectionProfile(this._capabilitiesService, this._model); + if (previousModel) { + previousModel.dispose(); + } if (this._inputModel && this._inputModel.options) { this.uiController.showUiComponent(input.container, this._inputModel.options.authTypeChanged); @@ -333,9 +336,12 @@ export class ConnectionDialogService implements IConnectionDialogService { private handleFillInConnectionInputs(connectionInfo: IConnectionProfile): void { this._connectionManagementService.addSavedPassword(connectionInfo).then(connectionWithPassword => { - let model = this.createModel(connectionWithPassword); - this._model = model; - this.uiController.fillInConnectionInputs(model); + if (this._model) { + this._model.dispose(); + } + this._model = this.createModel(connectionWithPassword); + + this.uiController.fillInConnectionInputs(this._model); }); this._connectionDialog.updateProvider(this._providerNameToDisplayNameMap[connectionInfo.providerName]); } @@ -349,6 +355,9 @@ export class ConnectionDialogService implements IConnectionDialogService { } private updateModelServerCapabilities(model: IConnectionProfile) { + if (this._model) { + this._model.dispose(); + } this._model = this.createModel(model); if (this._model.providerName) { this._currentProviderType = this._model.providerName; @@ -452,8 +461,10 @@ export class ConnectionDialogService implements IConnectionDialogService { this._connectionDialog.updateProvider(this._providerNameToDisplayNameMap[this._currentProviderType]); return new Promise(() => { - this._connectionDialog.open(this._connectionManagementService.getRecentConnections(params.providers).length > 0); + const recentConnections: ConnectionProfile[] = this._connectionManagementService.getRecentConnections(params.providers); + this._connectionDialog.open(recentConnections.length > 0); this.uiController.focusOnOpen(); + recentConnections.forEach(conn => conn.dispose()); }); } diff --git a/src/sql/workbench/services/connection/browser/connectionDialogWidget.ts b/src/sql/workbench/services/connection/browser/connectionDialogWidget.ts index f25a852d7f..a34e8258ba 100644 --- a/src/sql/workbench/services/connection/browser/connectionDialogWidget.ts +++ b/src/sql/workbench/services/connection/browser/connectionDialogWidget.ts @@ -321,10 +321,14 @@ export class ConnectionDialogWidget extends Modal { const actionProvider = this._instantiationService.createInstance(RecentConnectionActionsProvider); const controller = new RecentConnectionTreeController(leftClick, actionProvider, this._connectionManagementService, this._contextMenuService); actionProvider.onRecentConnectionRemoved(() => { - this.open(this._connectionManagementService.getRecentConnections().length > 0); + const recentConnections: ConnectionProfile[] = this._connectionManagementService.getRecentConnections(); + this.open(recentConnections.length > 0); + recentConnections.forEach(conn => conn.dispose()); }); controller.onRecentConnectionRemoved(() => { - this.open(this._connectionManagementService.getRecentConnections().length > 0); + const recentConnections: ConnectionProfile[] = this._connectionManagementService.getRecentConnections(); + this.open(recentConnections.length > 0); + recentConnections.forEach(conn => conn.dispose()); }); this._recentConnectionTree = TreeCreationUtils.createConnectionTree(treeContainer, this._instantiationService, controller);