Fix #4029 Ensure changeKernels always resolves, even in error states (#4488)

* Fix #4029 Ensure changeKernels always resolves, even in error states
This is necessary to unblock reverting the kernel on canceling  Python install
- startSession now correctly sets up kernel information, since a kernel is loaded there.
- Remove call to change kernel on session initialize. This isn't needed due to refactor
- Handle kernel change failure by attempting to fall back to old kernel
- ExtensionHost $startNewSession now ensures errors are sent across the wire.
- Update AttachTo and Kernel dropdowns so they handle kernel being available. This is needed since other changes mean the session is likely ready before these get going

* Fix to handle python cancel and load existing scenarios
- Made changes to handle failure flow when Python dialog is canceled
- Made changes to handle initial load fail. Kernel and Attach To dropdowns show No Kernel / None and you can choose a kernel
- Added error wrapping in ext host so that string errors make it across and aren't lost.

src/sql/parts/notebook/models/clientSession.ts

@@ -9,9 +9,9 @@
 'use strict';
 
 import { nb } from 'azdata';
-import * as nls from 'vs/nls';
 import { URI } from 'vs/base/common/uri';
 import { Event, Emitter } from 'vs/base/common/event';
+import { localize } from 'vs/nls';
 
 import { IClientSession, IKernelPreference, IClientSessionOptions } from './modelInterfaces';
 import { Deferred } from 'sql/base/common/promise';
@@ -70,12 +70,14 @@ export class ClientSession implements IClientSession {
 			await this.initializeSession();
 			await this.updateCachedKernelSpec();
 		} catch (err) {
-			this._errorMessage = notebookUtils.getErrorMessage(err);
+			this._errorMessage = notebookUtils.getErrorMessage(err) || localize('clientSession.unknownError', "An error occurred while starting the notebook session");
 		}
 		// Always resolving for now. It's up to callers to check for error case
 		this._isReady = true;
 		this._ready.resolve();
-		this._kernelChangeCompleted.resolve();
+		if (!this.isInErrorState && this._session && this._session.kernel) {
+			await this.notifyKernelChanged(undefined, this._session.kernel);
+		}
 	}
 
 	private async startServer(): Promise<void> {
@@ -83,7 +85,7 @@ export class ClientSession implements IClientSession {
 		if (serverManager && !serverManager.isStarted) {
 			await serverManager.startServer();
 			if (!serverManager.isStarted) {
-				throw new Error(nls.localize('ServerNotStarted', 'Server did not start for unknown reason'));
+				throw new Error(localize('ServerNotStarted', "Server did not start for unknown reason"));
 			}
 			this.isServerStarted = serverManager.isStarted;
 		} else {
@@ -116,7 +118,7 @@ export class ClientSession implements IClientSession {
 		} catch (err) {
 			// TODO move registration
 			if (err && err.response && err.response.status === 501) {
-				this.options.notificationService.warn(nls.localize('kernelRequiresConnection', 'Kernel {0} was not found. The default kernel will be used instead.', kernelName));
+				this.options.notificationService.warn(localize('kernelRequiresConnection', "Kernel {0} was not found. The default kernel will be used instead.", kernelName));
 				session = await this.notebookManager.sessionManager.startNew({
 					path: this.notebookUri.fsPath,
 					kernelName: undefined
@@ -246,6 +248,11 @@ export class ClientSession implements IClientSession {
 		this._isReady = kernel.isReady;
 		await this.updateCachedKernelSpec();
 		// Send resolution events to listeners
+		await this.notifyKernelChanged(oldKernel, newKernel);
+		return kernel;
+	}
+
+	private async notifyKernelChanged(oldKernel: nb.IKernel, newKernel: nb.IKernel): Promise<void> {
 		let changeArgs: nb.IKernelChangedArgs = {
 			oldValue: oldKernel,
 			newValue: newKernel
@@ -255,7 +262,6 @@ export class ClientSession implements IClientSession {
 		// Wait on connection configuration to complete before resolving full kernel change
 		this._kernelChangeCompleted.resolve();
 		this._kernelChangedEmitter.fire(changeArgs);
-		return kernel;
 	}
 
 	private async updateCachedKernelSpec(): Promise<void> {

src/sql/parts/notebook/models/notebookModel.ts

@@ -354,7 +354,7 @@ export class NotebookModel extends Disposable implements INotebookModel {
 				cellIndex: index
 			});
 		} else {
-			this.notifyError(localize('deleteCellFailed', 'Failed to delete cell.'));
+			this.notifyError(localize('deleteCellFailed', "Failed to delete cell."));
 		}
 	}
 
@@ -392,7 +392,7 @@ export class NotebookModel extends Disposable implements INotebookModel {
 		this._onErrorEmitter.fire({ message: error, severity: Severity.Error });
 	}
 
-	public async startSession(manager: INotebookManager, displayName?: string): Promise<void> {
+	public async startSession(manager: INotebookManager, displayName?: string, setErrorStateOnFail?: boolean): Promise<void> {
 		if (displayName) {
 			let standardKernel = this._notebookOptions.standardKernels.find(kernel => kernel.displayName === displayName);
 			this._defaultKernel = displayName ? { name: standardKernel.name, display_name: standardKernel.displayName } : this._defaultKernel;
@@ -406,7 +406,6 @@ export class NotebookModel extends Disposable implements INotebookModel {
 			});
 			if (!this._activeClientSession) {
 				this.updateActiveClientSession(clientSession);
-
 			}
 			let profile = new ConnectionProfile(this._notebookOptions.capabilitiesService, this.connectionProfile);
 
@@ -420,16 +419,31 @@ export class NotebookModel extends Disposable implements INotebookModel {
 				this._activeConnection = undefined;
 			}
 
+			clientSession.onKernelChanging(async (e) => {
+				await this.loadActiveContexts(e);
+			});
+			clientSession.statusChanged(async (session) => {
+				this._kernelsChangedEmitter.fire(session.kernel);
+			});
 			await clientSession.initialize();
 			// By somehow we have to wait for ready, otherwise may not be called for some cases.
 			await clientSession.ready;
-			if (clientSession.isInErrorState) {
-				this.setErrorState(clientSession.errorMessage);
-			} else {
-				this._onClientSessionReady.fire(clientSession);
-				// Once session is loaded, can use the session manager to retrieve useful info
-				this.loadKernelInfo(clientSession, this.defaultKernel.display_name);
+			if (clientSession.kernel) {
+				await clientSession.kernel.ready;
+				await this.updateKernelInfoOnKernelChange(clientSession.kernel);
 			}
+			if (clientSession.isInErrorState) {
+				if (setErrorStateOnFail) {
+					this.setErrorState(clientSession.errorMessage);
+				} else {
+					throw new Error(clientSession.errorMessage);
+				}
+			}
+			this._onClientSessionReady.fire(clientSession);
+			this._kernelChangedEmitter.fire({
+				oldValue: undefined,
+				newValue: clientSession.kernel
+			});
 		}
 	}
 
@@ -552,10 +566,57 @@ export class NotebookModel extends Disposable implements INotebookModel {
 		this.doChangeKernel(displayName, true);
 	}
 
-	public async doChangeKernel(displayName: string, mustSetProvider: boolean = true): Promise<void> {
-		if (mustSetProvider) {
-			await this.setProviderIdAndStartSession(displayName);
+	private async doChangeKernel(displayName: string, mustSetProvider: boolean = true, restoreOnFail: boolean = true): Promise<void> {
+		let oldDisplayName = this._activeClientSession && this._activeClientSession.kernel ? this._activeClientSession.kernel.name : undefined;
+		try {
+			let changeKernelNeeded = true;
+			if (mustSetProvider) {
+				changeKernelNeeded = await this.setProviderIdAndStartSession(displayName);
+			}
+			if (changeKernelNeeded) {
+				let spec = this.findSpec(displayName);
+				if (this._activeClientSession && this._activeClientSession.isReady) {
+					let kernel = await this._activeClientSession.changeKernel(spec, this._oldKernel);
+					try {
+						await kernel.ready;
+						await this.updateKernelInfoOnKernelChange(kernel);
+					} catch (err2) {
+						// TODO should we handle this in any way?
+						console.log(`doChangeKernel: ignoring error ${notebookUtils.getErrorMessage(err2)}`);
+					}
+				}
+			}
+		} catch (err) {
+			if (oldDisplayName && restoreOnFail) {
+				this.notifyError(localize('changeKernelFailedRetry', "Failed to change kernel. Kernel {0} will be used. Error was: {1}", oldDisplayName, notebookUtils.getErrorMessage(err)));
+				// Clear out previous kernel
+				let failedProviderId = this.tryFindProviderForKernel(displayName, true);
+				let oldProviderId = this.tryFindProviderForKernel(oldDisplayName, true);
+				if (failedProviderId !== oldProviderId) {
+					// We need to clear out the old kernel information so we switch providers. Otherwise in the SQL -> Jupyter -> SQL failure case,
+					// we would never reset the providers
+					this._oldKernel = undefined;
+				}
+				return this.doChangeKernel(oldDisplayName, mustSetProvider, false);
+			} else {
+				this.notifyError(localize('changeKernelFailed', "Failed to change kernel due to error: {0}", notebookUtils.getErrorMessage(err)));
+				this._kernelChangedEmitter.fire({
+					newValue: undefined,
+					oldValue: undefined
+				});
+			}
 		}
+		// Else no need to do anything
+	}
+
+	private async updateKernelInfoOnKernelChange(kernel: nb.IKernel) {
+		await this.updateKernelInfo(kernel);
+		if (kernel.info) {
+			this.updateLanguageInfo(kernel.info.language_info);
+		}
+	}
+
+	private findSpec(displayName: string) {
 		let spec = this.getKernelSpecFromDisplayName(displayName);
 		if (spec) {
 			// Ensure that the kernel we try to switch to is a valid kernel; if not, use the default
@@ -563,24 +624,11 @@ export class NotebookModel extends Disposable implements INotebookModel {
 			if (kernelSpecs && kernelSpecs.length > 0 && kernelSpecs.findIndex(k => k.display_name === spec.display_name) < 0) {
 				spec = kernelSpecs.find(spec => spec.name === this.notebookManager.sessionManager.specs.defaultKernel);
 			}
-		} else {
+		}
+		else {
 			spec = notebookConstants.sqlKernelSpec;
 		}
-		if (this._activeClientSession && this._activeClientSession.isReady) {
-			return this._activeClientSession.changeKernel(spec, this._oldKernel)
-				.then((kernel) => {
-					this.updateKernelInfo(kernel);
-					kernel.ready.then(() => {
-						if (kernel.info) {
-							this.updateLanguageInfo(kernel.info.language_info);
-						}
-					}, err => undefined);
-				}).catch((err) => {
-					this.notifyError(localize('changeKernelFailed', 'Failed to change kernel: {0}', notebookUtils.getErrorMessage(err)));
-					// TODO should revert kernels dropdown
-				});
-		}
-		return Promise.resolve();
+		return spec;
 	}
 
 	public async changeContext(server: string, newConnection?: IConnectionProfile, hideErrorMessage?: boolean): Promise<void> {
@@ -613,7 +661,7 @@ export class NotebookModel extends Disposable implements INotebookModel {
 				});
 		} catch (err) {
 			let msg = notebookUtils.getErrorMessage(err);
-			this.notifyError(localize('changeContextFailed', 'Changing context failed: {0}', msg));
+			this.notifyError(localize('changeContextFailed', "Changing context failed: {0}", msg));
 		}
 	}
 
@@ -631,17 +679,6 @@ export class NotebookModel extends Disposable implements INotebookModel {
 		}
 	}
 
-	private loadKernelInfo(clientSession: IClientSession, displayName: string): void {
-		clientSession.onKernelChanging(async (e) => {
-			await this.loadActiveContexts(e);
-		});
-		clientSession.statusChanged(async (session) => {
-			this._kernelsChangedEmitter.fire(session.kernel);
-		});
-
-		this.doChangeKernel(displayName, false);
-	}
-
 	// Get default language if saved in notebook file
 	// Otherwise, default to python
 	private getDefaultLanguageInfo(notebook: nb.INotebookContents): nb.ILanguageInfo {
@@ -703,7 +740,7 @@ export class NotebookModel extends Disposable implements INotebookModel {
 
 	private setErrorState(errMsg: string): void {
 		this._inErrorState = true;
-		let msg = localize('startSessionFailed', 'Could not start session: {0}', errMsg);
+		let msg = localize('startSessionFailed', "Could not start session: {0}", errMsg);
 		this.notifyError(msg);
 
 	}
@@ -730,7 +767,7 @@ export class NotebookModel extends Disposable implements INotebookModel {
 			}
 			await this.shutdownActiveSession();
 		} catch (err) {
-			this.notifyError(localize('shutdownError', 'An error occurred when closing the notebook: {0}', err));
+			this.notifyError(localize('shutdownError', "An error occurred when closing the notebook: {0}", err));
 		}
 	}
 
@@ -740,7 +777,7 @@ export class NotebookModel extends Disposable implements INotebookModel {
 				await this._activeClientSession.ready;
 			}
 			catch (err) {
-				this.notifyError(localize('shutdownClientSessionError', 'A client session error occurred when closing the notebook: {0}', err));
+				this.notifyError(localize('shutdownClientSessionError', "A client session error occurred when closing the notebook: {0}", err));
 			}
 			await this._activeClientSession.shutdown();
 			this.clearClientSessionListeners();
@@ -794,46 +831,39 @@ export class NotebookModel extends Disposable implements INotebookModel {
 	 * Set _providerId and start session if it is new provider
 	 * @param displayName Kernel dispay name
 	 */
-	private async setProviderIdAndStartSession(displayName: string): Promise<void> {
+	private async setProviderIdAndStartSession(displayName: string): Promise<boolean> {
 		if (displayName) {
 			if (this._activeClientSession && this._activeClientSession.isReady) {
 				this._oldKernel = this._activeClientSession.kernel;
-				let providerId = this.tryFindProviderForKernel(displayName);
+			}
+			let providerId = this.tryFindProviderForKernel(displayName);
 
-				if (providerId) {
-					if (providerId !== this._providerId) {
-						this._providerId = providerId;
-						this._onProviderIdChanged.fire(this._providerId);
+			if (providerId && providerId !== this._providerId) {
+				this._providerId = providerId;
+				this._onProviderIdChanged.fire(this._providerId);
 
-						await this.shutdownActiveSession();
-
-						try {
-							let manager = this.getNotebookManager(providerId);
-							if (manager) {
-								await this.startSession(manager, displayName);
-							} else {
-								throw new Error(localize('ProviderNoManager', "Can't find notebook manager for provider {0}", providerId));
-							}
-						}
-						catch (err) {
-							console.log(err);
-						}
-					} else {
-						console.log(`No provider found supporting the kernel: ${displayName}`);
-					}
+				await this.shutdownActiveSession();
+				let manager = this.getNotebookManager(providerId);
+				if (manager) {
+					await this.startSession(manager, displayName, false);
+				} else {
+					throw new Error(localize('ProviderNoManager', "Can't find notebook manager for provider {0}", providerId));
 				}
+				return true;
 			}
 		}
+		return false;
 	}
 
-	private tryFindProviderForKernel(displayName: string) {
+	private tryFindProviderForKernel(displayName: string, alwaysReturnId: boolean = false) {
 		if (!displayName) {
 			return undefined;
 		}
 		let standardKernel = this.getStandardKernelFromDisplayName(displayName);
-		if (standardKernel && this._oldKernel && this._oldKernel.name !== standardKernel.name) {
-			if (this._kernelDisplayNameToNotebookProviderIds.has(displayName)) {
-				return this._kernelDisplayNameToNotebookProviderIds.get(displayName);
+		if (standardKernel) {
+			let providerId = this._kernelDisplayNameToNotebookProviderIds.get(displayName);
+			if (alwaysReturnId || (!this._oldKernel || this._oldKernel.name !== standardKernel.name)) {
+				return providerId;
 			}
 		}
 		return undefined;
@@ -883,7 +913,7 @@ export class NotebookModel extends Disposable implements INotebookModel {
 		};
 	}
 
-	onCellChange(cell: CellModel, change: NotebookChangeType): void {
+	onCellChange(cell: ICellModel, change: NotebookChangeType): void {
 		let changeInfo: NotebookContentChange = {
 			changeType: change,
 			cells: [cell]

src/sql/parts/notebook/notebook.component.ts

@@ -248,7 +248,7 @@ export class NotebookComponent extends AngularDisposable implements OnInit, OnDe
 		this._model = this._register(model);
 		this.updateToolbarComponents(this._model.trustedMode);
 		this._modelRegisteredDeferred.resolve(this._model);
-		await model.startSession(this.model.notebookManager);
+		await model.startSession(this.model.notebookManager, undefined, true);
 		this.detectChanges();
 	}
 

src/sql/parts/notebook/notebookActions.ts

@@ -235,18 +235,24 @@ export class KernelsDropdown extends SelectBox {
 		this._register(this.model.kernelChanged((changedArgs: azdata.nb.IKernelChangedArgs) => {
 			this.updateKernel(changedArgs.newValue);
 		}));
+		let kernel = this.model.clientSession && this.model.clientSession.kernel;
+		this.updateKernel(kernel);
 	}
 
 	// Update SelectBox values
 	public updateKernel(kernel: azdata.nb.IKernel) {
-		if (kernel) {
+		let kernels: string[] = this.model.standardKernelsDisplayName();
+		if (kernel && kernel.isReady) {
 			let standardKernel = this.model.getStandardKernelFromName(kernel.name);
 
-			let kernels: string[] = this.model.standardKernelsDisplayName();
 			if (kernels && standardKernel) {
 				let index = kernels.findIndex((kernel => kernel === standardKernel.displayName));
 				this.setOptions(kernels, index);
 			}
+		} else if (this.model.clientSession.isInErrorState) {
+			let noKernelName = localize('noKernel', "No Kernel");
+			kernels.unshift(noKernelName);
+			this.setOptions(kernels, 0);
 		}
 	}
 
@@ -283,22 +289,26 @@ export class AttachToDropdown extends SelectBox {
 	public updateModel(model: INotebookModel): void {
 		this.model = model as NotebookModel;
 		this._register(model.contextsChanged(() => {
-			let kernelDisplayName: string = this.getKernelDisplayName();
-			if (kernelDisplayName) {
-				this.loadAttachToDropdown(this.model, kernelDisplayName);
-			}
+			this.handleContextsChanged();
 		}));
 		this._register(this.model.contextsLoading(() => {
 			this.setOptions([msgLoadingContexts], 0);
 		}));
+		this.handleContextsChanged();
+	}
+
+	private handleContextsChanged(showSelectConnection?: boolean) {
+		let kernelDisplayName: string = this.getKernelDisplayName();
+		if (kernelDisplayName) {
+			this.loadAttachToDropdown(this.model, kernelDisplayName, showSelectConnection);
+		} else if (this.model.clientSession.isInErrorState) {
+			this.setOptions([localize('noContextAvailable', "None")], 0);
+		}
 	}
 
 	private updateAttachToDropdown(model: INotebookModel): void {
 		model.onValidConnectionSelected(validConnection => {
-			let kernelDisplayName: string = this.getKernelDisplayName();
-			if (kernelDisplayName) {
-				this.loadAttachToDropdown(this.model, kernelDisplayName, !validConnection);
-			}
+			this.handleContextsChanged(!validConnection);
 		});
 	}
 

src/sql/workbench/api/node/extHostNotebook.ts

@@ -81,23 +81,27 @@ export class ExtHostNotebook implements ExtHostNotebookShape {
 
 	$startNewSession(managerHandle: number, options: azdata.nb.ISessionOptions): Thenable<INotebookSessionDetails> {
 		return this._withSessionManager(managerHandle, async (sessionManager) => {
-			let session = await sessionManager.startNew(options);
-			let sessionId = this._addNewAdapter(session);
-			let kernelDetails: INotebookKernelDetails = undefined;
-			if (session.kernel) {
-				kernelDetails = this.saveKernel(session.kernel);
+			try {
+				let session = await sessionManager.startNew(options);
+				let sessionId = this._addNewAdapter(session);
+				let kernelDetails: INotebookKernelDetails = undefined;
+				if (session.kernel) {
+					kernelDetails = this.saveKernel(session.kernel);
+				}
+				let details: INotebookSessionDetails = {
+					sessionId: sessionId,
+					id: session.id,
+					path: session.path,
+					name: session.name,
+					type: session.type,
+					status: session.status,
+					canChangeKernels: session.canChangeKernels,
+					kernelDetails: kernelDetails
+				};
+				return details;
+			} catch (error) {
+				throw typeof(error) === 'string' ? new Error(error) : error;
 			}
-			let details: INotebookSessionDetails = {
-				sessionId: sessionId,
-				id: session.id,
-				path: session.path,
-				name: session.name,
-				type: session.type,
-				status: session.status,
-				canChangeKernels: session.canChangeKernels,
-				kernelDetails: kernelDetails
-			};
-			return details;
 		});
 	}
 
@@ -267,7 +271,16 @@ export class ExtHostNotebook implements ExtHostNotebookShape {
 		if (manager === undefined) {
 			return TPromise.wrapError<R>(new Error(localize('errNoManager', 'No Manager found')));
 		}
-		return TPromise.wrap(callback(manager));
+		return TPromise.wrap(this.callbackWithErrorWrap<R>(callback, manager));
+	}
+
+	private async callbackWithErrorWrap<R>(callback: (manager: NotebookManagerAdapter) => R | PromiseLike<R>, manager: NotebookManagerAdapter): Promise<R> {
+		try {
+			let value = await callback(manager);
+			return value;
+		} catch (error) {
+			throw typeof(error) === 'string' ? new Error(error) : error;
+		}
 	}
 
 	private _withServerManager<R>(handle: number, callback: (manager: azdata.nb.ServerManager) => R | PromiseLike<R>): TPromise<R> {