Fixing async server tree error handling and removing timeout. (#22955)

* Fixing async server tree issues and removing timeout

* removing empty results for connection errors

* Fixing error message fetching

* Update src/sql/workbench/services/objectExplorer/browser/asyncServerTreeDataSource.ts

Co-authored-by: Charles Gagnon <chgagnon@microsoft.com>

---------

Co-authored-by: Charles Gagnon <chgagnon@microsoft.com>

src/sql/workbench/contrib/objectExplorer/common/serverGroup.contribution.ts

@@ -41,7 +41,6 @@ const serverGroupConfig: IConfigurationNode = {
 	}
 };
 
-export const NODE_EXPANSION_CONFIG = 'serverTree.nodeExpansionTimeout';
 export const USE_ASYNC_SERVER_TREE_CONFIG = 'serverTree.useAsyncServerTree';
 const serverTreeConfig: IConfigurationNode = {
 	'id': 'serverTree',
@@ -52,12 +51,6 @@ const serverTreeConfig: IConfigurationNode = {
 			'type': 'boolean',
 			'default': true,
 			'description': localize('serverTree.useAsyncServerTree', "Use the new async server tree for the Servers view and Connection Dialog with support for new features such as dynamic node filtering. Requires a restart to take effect.")
-		},
-		'serverTree.nodeExpansionTimeout': {
-			'type': 'number',
-			'default': '45',
-			'description': localize('serverTree.nodeExpansionTimeout', "The timeout in seconds for expanding a node in the Servers view"),
-			'minimum': 1
 		}
 	}
 };

src/sql/workbench/services/objectExplorer/browser/asyncServerTree.ts

@@ -151,3 +151,10 @@ export class AsyncServerTree extends WorkbenchAsyncDataTree<ConnectionProfileGro
 }
 
 export type ServerTreeElement = ConnectionProfile | ConnectionProfileGroup | TreeNode;
+
+
+export class ConnectionError extends Error {
+	constructor(message: string, public connection: ConnectionProfile) {
+		super(message);
+	}
+}

src/sql/workbench/services/objectExplorer/browser/asyncServerTreeDataSource.ts

@@ -12,8 +12,9 @@ import { IConnectionManagementService } from 'sql/platform/connection/common/con
 import Severity from 'vs/base/common/severity';
 import { IErrorMessageService } from 'sql/platform/errorMessage/common/errorMessageService';
 import { IAsyncDataSource } from 'vs/base/browser/ui/tree/tree';
-import { ServerTreeElement } from 'sql/workbench/services/objectExplorer/browser/asyncServerTree';
+import { ConnectionError, ServerTreeElement } from 'sql/workbench/services/objectExplorer/browser/asyncServerTree';
 import { IConfigurationService } from 'vs/platform/configuration/common/configuration';
+import { getErrorMessage } from 'vs/base/common/errors';
 
 /**
  * Implements the DataSource(that returns a parent/children of an element) for the server tree
@@ -51,18 +52,16 @@ export class AsyncServerTreeDataSource implements IAsyncDataSource<ConnectionPro
 			} else if (element instanceof ConnectionProfileGroup) {
 				return element.getChildren();
 			} else if (element instanceof TreeNode) {
-				if (element.children) {
-					return element.children;
-				} else {
-					return await this._objectExplorerService.resolveTreeNodeChildren(element.getSession()!, element);
-				}
+				return await this._objectExplorerService.resolveTreeNodeChildren(element.getSession()!, element);
 			}
 		} catch (err) {
+			const errorMessage = getErrorMessage(err);
 			if (element instanceof TreeNode) {
-				element.errorStateMessage = err.message ?? err;
+				element.errorStateMessage = errorMessage;
 			}
-			if (err.message) {
-				this.showError(err.message);
+			// In case of connection profile, we won't show the error here and let the connection service handle it.
+			if (errorMessage && !(err instanceof ConnectionError)) {
+				this.showError(errorMessage);
 			}
 
 			throw err;

src/sql/workbench/services/objectExplorer/browser/objectExplorerService.ts

@@ -24,9 +24,6 @@ import { ITree } from 'sql/base/parts/tree/browser/tree';
 import { AsyncServerTree, ServerTreeElement } from 'sql/workbench/services/objectExplorer/browser/asyncServerTree';
 import { mssqlProviderName } from 'sql/platform/connection/common/constants';
 import { ObjectExplorerRequestStatus } from 'sql/workbench/services/objectExplorer/browser/treeSelectionHandler';
-import { IConfigurationService } from 'vs/platform/configuration/common/configuration';
-import { INotificationService } from 'vs/platform/notification/common/notification';
-import { NODE_EXPANSION_CONFIG } from 'sql/workbench/contrib/objectExplorer/common/serverGroup.contribution';
 
 export const SERVICE_ID = 'ObjectExplorerService';
 
@@ -190,8 +187,6 @@ export class ObjectExplorerService implements IObjectExplorerService {
 		@IAdsTelemetryService private _telemetryService: IAdsTelemetryService,
 		@ICapabilitiesService private _capabilitiesService: ICapabilitiesService,
 		@ILogService private logService: ILogService,
-		@IConfigurationService private _configurationService: IConfigurationService,
-		@INotificationService private _notificationService: INotificationService
 	) {
 		this._onUpdateObjectExplorerNodes = new Emitter<ObjectExplorerNodeEventArgs>();
 		this._activeObjectExplorerNodes = {};
@@ -298,27 +293,8 @@ export class ObjectExplorerService implements IObjectExplorerService {
 					this._connectionsWaitingForSession.delete(connection.id);
 				}
 				createNewSessionListener.dispose();
-				clearTimeout(timeoutHandle);
 			}
-			const onTimeout = () => {
-				if (!this._sessions[sessionId]) {
-					this.logService.error(`Timed out waiting for session ${sessionId} to be created.
-					This has probably happened because OE service did not recieve a response for createNewSession from the provider.`);
-					reject(new Error(
-						nls.localize('objectExplorerMissingSession',
-							'Timed out waiting for session {0} to be created. This has probably happened because OE service did not recieve a response for createNewSession from the provider.', sessionId)));
-				} else {
-					this.logService.error(`Timeout waiting for session ${sessionId} to be created for connection "${connection.title}".
-					This has probably happened because OE service did not recieve a response for createNewSession from the provider for connection."${connection.title}`);
-					reject(new Error(nls.localize(
-						'objectExplorerMissingConnectionForSession',
-						'Timeout waiting for session {0} to be created for connection "{1}". This has probably happened because OE service did not recieve a response for createNewSession from the provider for connection "{1}"', sessionId, connection.title
-					)));
-				}
 
-				cleanup();
-			}
-			const timeoutHandle = setTimeout(onTimeout, this.getObjectExplorerTimeout() * 1000);
 			const createNewSessionListener = this._onCreateNewSession.event((response) => {
 				checkSessionAndConnection();
 			});
@@ -510,7 +486,6 @@ export class ObjectExplorerService implements IObjectExplorerService {
 					const resolveExpansion = () => {
 						resolve(self.mergeResults(allProviders, resultMap, node.nodePath));
 						// Have to delete it after get all responses otherwise couldn't find session for not the first response
-						clearTimeout(expansionTimeout);
 						if (newRequest) {
 							delete self._sessions[session.sessionId!].nodes[node.nodePath];
 							this.logService.trace(`Deleted node ${node.nodePath} from session ${session.sessionId}`);
@@ -529,19 +504,6 @@ export class ObjectExplorerService implements IObjectExplorerService {
 						}
 					});
 
-					const expansionTimeout = setTimeout(() => {
-						/**
-						 * If we don't get a response back from all the providers in specified expansion timeout seconds then we assume
-						 * it's not going to respond and resolve the promise with the results we have so far
-						 */
-						if (resultMap.size !== allProviders.length) {
-							const missingProviders = allProviders.filter(p => !resultMap.has(p.providerId));
-							this.logService.warn(`${session.sessionId}: Node expansion timed out for node ${node.nodePath} for providers ${missingProviders.map(p => p.providerId).join(', ')}`);
-							this._notificationService.error(nls.localize('nodeExpansionTimeout', "Node expansion timed out for node {0} for providers {1}", node.nodePath, missingProviders.map(p => p.providerId).join(', ')));
-						}
-						resolveExpansion();
-					}, this.getObjectExplorerTimeout() * 1000);
-
 					self._sessions[session.sessionId!].nodes[node.nodePath].expandEmitter.event((expandResult: NodeExpandInfoWithProviderId) => {
 						if (expandResult && expandResult.providerId) {
 							this.logService.trace(`${session.sessionId}: Received expand result for node ${node.nodePath} from provider ${expandResult.providerId}`);
@@ -600,8 +562,12 @@ export class ObjectExplorerService implements IObjectExplorerService {
 		});
 	}
 
-	private mergeResults(allProviders: azdata.ObjectExplorerProviderBase[], resultMap: Map<string, azdata.ObjectExplorerExpandInfo>, nodePath: string): azdata.ObjectExplorerExpandInfo | undefined {
-		let finalResult: azdata.ObjectExplorerExpandInfo | undefined = undefined;
+	private mergeResults(allProviders: azdata.ObjectExplorerProviderBase[], resultMap: Map<string, azdata.ObjectExplorerExpandInfo>, nodePath: string): azdata.ObjectExplorerExpandInfo {
+		let finalResult: azdata.ObjectExplorerExpandInfo | undefined = {
+			sessionId: undefined,
+			nodePath: nodePath,
+			nodes: []
+		};
 		let allNodes: azdata.NodeInfo[] = [];
 		let errorNode: azdata.NodeInfo = {
 			nodePath: nodePath,
@@ -620,12 +586,14 @@ export class ObjectExplorerService implements IObjectExplorerService {
 			if (resultMap.has(provider.providerId)) {
 				let result = resultMap.get(provider.providerId);
 				if (result) {
+
 					if (!result.errorMessage) {
 						finalResult = result;
 						if (result.nodes !== undefined && result.nodes) {
 							allNodes = allNodes.concat(result.nodes);
 						}
 					} else {
+						finalResult.sessionId = result.sessionId;
 						errorMessages.push(result.errorMessage);
 					}
 				}
@@ -639,8 +607,11 @@ export class ObjectExplorerService implements IObjectExplorerService {
 				errorNode.errorMessage = errorMessages.join('\n');
 				errorNode.label = errorNode.errorMessage;
 				allNodes = [errorNode].concat(allNodes);
+				this._onUpdateObjectExplorerNodes.fire({
+					connection: undefined,
+					errorMessage: errorNode.errorMessage
+				});
 			}
-
 			finalResult.nodes = allNodes;
 		}
 		return finalResult;
@@ -1049,13 +1020,6 @@ export class ObjectExplorerService implements IObjectExplorerService {
 		return currentNode;
 	}
 
-	/**
-	 * returns object explorer timeout in seconds.
-	 */
-	public getObjectExplorerTimeout(): number {
-		return this._configurationService.getValue<number>(NODE_EXPANSION_CONFIG);
-	}
-
 	private getTreeNodeCacheKey(node: azdata.NodeInfo | TreeNode): string {
 		return node.nodePath;
 	}

src/sql/workbench/services/objectExplorer/browser/treeUpdateUtils.ts

@@ -12,11 +12,9 @@ import { NodeType } from 'sql/workbench/services/objectExplorer/common/nodeType'
 
 import { TreeNode } from 'sql/workbench/services/objectExplorer/common/treeNode';
 import { Disposable, isDisposable } from 'vs/base/common/lifecycle';
-import { onUnexpectedError } from 'vs/base/common/errors';
-import { AsyncServerTree, ServerTreeElement } from 'sql/workbench/services/objectExplorer/browser/asyncServerTree';
+import { getErrorMessage, onUnexpectedError } from 'vs/base/common/errors';
+import { AsyncServerTree, ConnectionError as AsyncTreeConnectionError, ServerTreeElement } from 'sql/workbench/services/objectExplorer/browser/asyncServerTree';
 import { ObjectExplorerRequestStatus } from 'sql/workbench/services/objectExplorer/browser/treeSelectionHandler';
-import * as nls from 'vs/nls';
-import { NODE_EXPANSION_CONFIG } from 'sql/workbench/contrib/objectExplorer/common/serverGroup.contribution';
 import { IConfigurationService } from 'vs/platform/configuration/common/configuration';
 
 export interface IExpandableTree extends ITree {
@@ -184,52 +182,58 @@ export class TreeUpdateUtils {
 		options: IConnectionCompletionOptions,
 		connectionManagementService: IConnectionManagementService,
 		tree: AsyncServerTree | ITree | undefined): Promise<ConnectionProfile | undefined> {
-		if (!connectionManagementService.isProfileConnected(connection)) {
-			// don't try to reconnect if currently connecting
-			if (connectionManagementService.isProfileConnecting(connection)) {
-				return undefined;
 
-				// else if we aren't connected or connecting then try to connect
-			} else {
-				let callbacks: IConnectionCallbacks | undefined;
-				if (tree instanceof AsyncServerTree) {
-					callbacks = {
-						onConnectStart: () => { },
-						onConnectReject: () => { },
-						onConnectSuccess: () => { },
-						onDisconnect: () => { },
-						onConnectCanceled: () => { },
-					};
-				} else if (tree) {
-					// Show the spinner in OE by adding the 'loading' trait to the connection, and set up callbacks to hide the spinner
-					tree.addTraits('loading', [connection]);
-					let rejectOrCancelCallback = () => {
-						tree.collapse(connection);
-						tree.removeTraits('loading', [connection]);
-					};
-					callbacks = {
-						onConnectStart: () => { },
-						onConnectReject: rejectOrCancelCallback,
-						onConnectSuccess: () => tree.removeTraits('loading', [connection]),
-						onDisconnect: () => { },
-						onConnectCanceled: rejectOrCancelCallback,
-					};
-				}
+		try {
+			if (!connectionManagementService.isProfileConnected(connection)) {
+				// don't try to reconnect if currently connecting
+				if (connectionManagementService.isProfileConnecting(connection)) {
+					return undefined;
 
-				const result = await connectionManagementService.connect(connection, undefined, options, callbacks);
-				if (result.connected) {
-					let existingConnection = connectionManagementService.findExistingConnection(connection);
-					return existingConnection;
+					// else if we aren't connected or connecting then try to connect
 				} else {
-					throw new Error(result.errorMessage);
+					let callbacks: IConnectionCallbacks | undefined;
+					if (tree instanceof AsyncServerTree) {
+						callbacks = {
+							onConnectStart: () => { },
+							onConnectReject: () => { },
+							onConnectSuccess: () => { },
+							onDisconnect: () => { },
+							onConnectCanceled: () => { },
+						};
+					} else if (tree) {
+						// Show the spinner in OE by adding the 'loading' trait to the connection, and set up callbacks to hide the spinner
+						tree.addTraits('loading', [connection]);
+						let rejectOrCancelCallback = () => {
+							tree.collapse(connection);
+							tree.removeTraits('loading', [connection]);
+						};
+						callbacks = {
+							onConnectStart: () => { },
+							onConnectReject: rejectOrCancelCallback,
+							onConnectSuccess: () => tree.removeTraits('loading', [connection]),
+							onDisconnect: () => { },
+							onConnectCanceled: rejectOrCancelCallback,
+						};
+					}
+
+					const result = await connectionManagementService.connect(connection, undefined, options, callbacks);
+					if (result.connected) {
+						let existingConnection = connectionManagementService.findExistingConnection(connection);
+						return existingConnection;
+					} else {
+						throw new Error(result.errorMessage);
+					}
 				}
+			} else {
+				let existingConnection = connectionManagementService.findExistingConnection(connection);
+				if (options && options.showDashboard) {
+					await connectionManagementService.showDashboard(connection);
+				}
+				return existingConnection;
 			}
-		} else {
-			let existingConnection = connectionManagementService.findExistingConnection(connection);
-			if (options && options.showDashboard) {
-				await connectionManagementService.showDashboard(connection);
-			}
-			return existingConnection;
+		} catch (e) {
+			// Since the connection dialog handles connection errors, we don't need to do anything here
+			throw new AsyncTreeConnectionError(getErrorMessage(e), connection);
 		}
 	}
 
@@ -249,6 +253,7 @@ export class TreeUpdateUtils {
 		objectExplorerService: IObjectExplorerService,
 		tree: AsyncServerTree | ITree | undefined,
 		requestStatus?: ObjectExplorerRequestStatus | undefined): Promise<boolean> {
+
 		const connectedConnection = await TreeUpdateUtils.connectIfNotConnected(connection, options, connectionManagementService, tree);
 		if (connectedConnection) {
 			// append group ID and original display name to build unique OE session ID
@@ -305,28 +310,18 @@ export class TreeUpdateUtils {
 				return rootNode.children ?? [];
 			} else {
 				const options: IConnectionCompletionOptions = {
+					params: undefined,
 					saveTheConnection: true,
 					showConnectionDialogOnError: true,
 					showFirewallRuleOnError: true,
 					showDashboard: false
 				};
-				const expansionTimeoutValueSec = configurationService.getValue<number>(NODE_EXPANSION_CONFIG);
 				// Need to wait for the OE service to update its nodes in order to resolve the children
 				const nodesUpdatedPromise = new Promise<void>((resolve, reject) => {
 					// Clean up timeout and listener
 					const cleanup = () => {
-						clearTimeout(nodeUpdateTimer);
 						nodesUpdatedListener.dispose();
 					}
-
-					// If the node update takes too long, reject the promise
-					const nodeUpdateTimeout = () => {
-						cleanup();
-						reject(new Error(nls.localize('objectExplorerTimeout', "Object Explorer expansion timed out for '{0}'", connection.databaseName)));
-					}
-					const nodeUpdateTimer = setTimeout(nodeUpdateTimeout, expansionTimeoutValueSec * 1000);
-
-
 					const nodesUpdatedListener = objectExplorerService.onUpdateObjectExplorerNodes(e => {
 						if (e.connection && e.connection.id === connection.id) {
 							if (e.errorMessage) {

src/sql/workbench/services/objectExplorer/test/browser/objectExplorerService.test.ts

@@ -288,9 +288,7 @@ suite('SQL Object Explorer Service tests', () => {
 			connectionManagementService.object,
 			new NullAdsTelemetryService(),
 			capabilitiesService,
-			logService,
-			configurationService.object,
-			notificationService.object);
+			logService);
 
 		objectExplorerService.registerProvider(mssqlProviderName, sqlOEProvider.object);
 		sqlOEProvider.setup(x => x.createNewSession(TypeMoq.It.is<azdata.ConnectionInfo>(x => x.options['serverName'] === connection.serverName))).returns(() => new Promise<any>((resolve) => {