Merge pull request #8174 from blink1073/slow-kernel-indicator

Better handling of slow kernel startup

packages/apputils/src/sessioncontext.tsx

@@ -122,7 +122,7 @@ export interface ISessionContext extends IObservableDisposable {
   kernelPreference: ISessionContext.IKernelPreference;
 
   /**
-   * The sensible display name for the kernel, or 'No Kernel'
+   * The sensible display name for the kernel, or Private.NO_KERNEL
    *
    * #### Notes
    * This is at this level since the underlying kernel connection does not
@@ -441,16 +441,35 @@ export class SessionContext implements ISessionContext {
    */
   get kernelDisplayName(): string {
     let kernel = this.session?.kernel;
+    if (this._pendingKernelName === Private.NO_KERNEL) {
+      return Private.NO_KERNEL;
+    }
     if (
       !kernel &&
       !this.isReady &&
       this.kernelPreference.canStart !== false &&
       this.kernelPreference.shouldStart !== false
     ) {
-      return 'Kernel';
+      let name =
+        this._pendingKernelName ||
+        SessionContext.getDefaultKernel({
+          specs: this.specsManager.specs,
+          sessions: this.sessionManager.running(),
+          preference: this.kernelPreference
+        }) ||
+        '';
+      if (name) {
+        name = this.specsManager.specs!.kernelspecs[name]!.display_name;
+        return name;
+      }
+      return Private.NO_KERNEL;
+    }
+    if (this._pendingKernelName) {
+      return this.specsManager.specs!.kernelspecs[this._pendingKernelName]!
+        .display_name;
     }
     if (!kernel) {
-      return 'No Kernel!';
+      return Private.NO_KERNEL;
     }
     return (
       this.specsManager.specs?.kernelspecs[kernel.name]?.display_name ??
@@ -467,6 +486,14 @@ export class SessionContext implements ISessionContext {
    */
   get kernelDisplayStatus(): ISessionContext.KernelDisplayStatus {
     let kernel = this.session?.kernel;
+    if (this._pendingKernelName === Private.NO_KERNEL) {
+      return 'idle';
+    }
+
+    if (!kernel && this._pendingKernelName) {
+      return 'initializing';
+    }
+
     if (
       !kernel &&
       !this.isReady &&
@@ -517,7 +544,7 @@ export class SessionContext implements ISessionContext {
     if (this._session) {
       if (this.kernelPreference.shutdownOnDispose) {
         // Fire and forget the session shutdown request
-        this._session.shutdown().catch(reason => {
+        this.sessionManager.shutdown(this._session.id).catch(reason => {
           console.error(`Kernel not shut down ${reason}`);
         });
       }
@@ -542,10 +569,13 @@ export class SessionContext implements ISessionContext {
   async changeKernel(
     options: Partial<Kernel.IModel> = {}
   ): Promise<Kernel.IKernelConnection | null> {
-    await this.initialize();
     if (this.isDisposed) {
       throw new Error('Disposed');
     }
+    // Wait for the initialization method to try
+    // and start its kernel first to ensure consistent
+    // ordering.
+    await this._initStarted.promise;
     return this._changeKernel(options);
   }
 
@@ -555,7 +585,13 @@ export class SessionContext implements ISessionContext {
    * @returns A promise that resolves when the session is shut down.
    */
   async shutdown(): Promise<void> {
-    return this._session?.shutdown();
+    if (this.isDisposed || !this._initializing) {
+      return;
+    }
+    await this._initStarted.promise;
+    this._pendingSessionRequest = '';
+    this._pendingKernelName = Private.NO_KERNEL;
+    return this._shutdownSession();
   }
 
   /**
@@ -576,8 +612,26 @@ export class SessionContext implements ISessionContext {
       return this._initPromise.promise;
     }
     this._initializing = true;
+    const needsSelection = await this._initialize();
+    if (!needsSelection) {
+      this._isReady = true;
+      this._ready.resolve(undefined);
+    }
+    if (!this._pendingSessionRequest) {
+      this._initStarted.resolve(void 0);
+    }
+    this._initPromise.resolve(needsSelection);
+    return needsSelection;
+  }
+
+  /**
+   * Inner initialize function that doesn't handle promises.
+   * This makes it easier to consolidate promise handling logic.
+   */
+  async _initialize(): Promise<boolean> {
     let manager = this.sessionManager;
     await manager.ready;
+    await manager.refreshRunning();
     let model = find(manager.running(), item => {
       return item.path === this._path;
     });
@@ -590,13 +644,30 @@ export class SessionContext implements ISessionContext {
         return Promise.reject(err);
       }
     }
-    const needsSelection = await this._startIfNecessary();
-    if (!needsSelection) {
-      this._isReady = true;
-      this._ready.resolve(undefined);
-    }
-    this._initPromise.resolve(needsSelection);
-    return needsSelection;
+
+    return await this._startIfNecessary();
+  }
+
+  /**
+   * Shut down the current session.
+   */
+  private async _shutdownSession(): Promise<void> {
+    const session = this._session;
+    this._session = null;
+    const kernel = session?.kernel || null;
+    this._kernelChanged.emit({
+      name: 'kernel',
+      oldValue: kernel,
+      newValue: null
+    });
+    this._statusChanged.emit('unknown');
+    await session?.shutdown();
+    session?.dispose();
+    this._sessionChanged.emit({
+      name: 'session',
+      oldValue: session,
+      newValue: null
+    });
   }
 
   /**
@@ -647,40 +718,53 @@ export class SessionContext implements ISessionContext {
    * Change the kernel.
    */
   private async _changeKernel(
-    options: Partial<Kernel.IModel> = {}
+    model: Partial<Kernel.IModel> = {},
+    isInit = false
   ): Promise<Kernel.IKernelConnection | null> {
-    if (this.isDisposed) {
-      throw new Error('Disposed');
+    if (model.name) {
+      this._pendingKernelName = model.name;
     }
-    let session = this._session;
-    if (session && session.kernel?.status !== 'dead') {
-      try {
-        return session.changeKernel(options);
-      } catch (err) {
-        void this._handleSessionError(err);
-        throw err;
-      }
+
+    if (this._session) {
+      await this._shutdownSession();
     } else {
-      return this._startSession(options);
+      this._kernelChanged.emit({
+        name: 'kernel',
+        oldValue: null,
+        newValue: null
+      });
     }
-  }
 
-  /**
-   * Start a session and set up its signals.
-   */
-  private async _startSession(
-    model: Partial<Kernel.IModel> = {}
-  ): Promise<Kernel.IKernelConnection | null> {
-    if (this.isDisposed) {
-      throw 'Client session is disposed.';
+    // Guarantee that the initialized kernel
+    // will be started first.
+    if (!this._pendingSessionRequest) {
+      this._initStarted.resolve(void 0);
     }
+    const requestId = (this._pendingSessionRequest = UUID.uuid4());
     try {
+      // Use a UUID for the path to overcome a race condition on the server
+      // where it will re-use a session for a given path but only after
+      // the kernel finishes starting.
+      // We later switch to the real path below.
+      this._statusChanged.emit('starting');
       const session = await this.sessionManager.startNew({
-        path: this._path,
+        path: requestId,
         type: this._type,
         name: this._name,
         kernel: model
       });
+      // Handle a preempt.
+      if (this._pendingSessionRequest !== session.path) {
+        await session.shutdown();
+        session.dispose();
+        return null;
+      }
+      // Change to the real path.
+      await session.setPath(this._path);
+
+      if (this._session) {
+        await this._shutdownSession();
+      }
       return this._handleNewSession(session);
     } catch (err) {
       void this._handleSessionError(err);
@@ -706,6 +790,7 @@ export class SessionContext implements ISessionContext {
     }
     this._session = session;
     this._prevKernelName = session.kernel?.name ?? '';
+    this._pendingKernelName = '';
 
     session.disposed.connect(this._onSessionDisposed, this);
     session.propertyChanged.connect(this._onPropertyChanged, this);
@@ -730,17 +815,17 @@ export class SessionContext implements ISessionContext {
 
     // Any existing session/kernel connection was disposed above when the session was
     // disposed, so the oldValue should be null.
+    this._sessionChanged.emit({
+      name: 'session',
+      oldValue: null,
+      newValue: session
+    });
     this._kernelChanged.emit({
       oldValue: null,
       newValue: session.kernel,
       name: 'kernel'
     });
-    this._sessionChanged.emit({
-      oldValue: null,
-      newValue: session,
-      name: 'session'
-    });
-
+    this._statusChanged.emit(session?.kernel?.status || 'unknown');
     return session.kernel;
   }
 
@@ -785,7 +870,6 @@ export class SessionContext implements ISessionContext {
    */
   private _onSessionDisposed(): void {
     if (this._session) {
-      this._session.dispose();
       const oldValue = this._session;
       this._session = null;
       const newValue = this._session;
@@ -894,6 +978,7 @@ export class SessionContext implements ISessionContext {
   private _session: Session.ISessionConnection | null = null;
   private _ready = new PromiseDelegate<void>();
   private _initializing = false;
+  private _initStarted = new PromiseDelegate<void>();
   private _initPromise = new PromiseDelegate<boolean>();
   private _isReady = false;
   private _kernelChanged = new Signal<
@@ -912,12 +997,15 @@ export class SessionContext implements ISessionContext {
   private _connectionStatusChanged = new Signal<this, Kernel.ConnectionStatus>(
     this
   );
+
   private _iopubMessage = new Signal<this, KernelMessage.IIOPubMessage>(this);
   private _unhandledMessage = new Signal<this, KernelMessage.IMessage>(this);
   private _propertyChanged = new Signal<this, 'path' | 'name' | 'type'>(this);
   private _dialog: Dialog<any> | null = null;
   private _setBusy: (() => IDisposable) | undefined;
   private _busyDisposable: IDisposable | null = null;
+  private _pendingKernelName = '';
+  private _pendingSessionRequest = '';
 }
 
 /**
@@ -1003,11 +1091,12 @@ export const sessionContextDialogs: ISessionContext.IDialogs = {
     if (sessionContext.isDisposed) {
       return Promise.resolve();
     }
+
     // If there is no existing kernel, offer the option
     // to keep no kernel.
     let label = 'Cancel';
-    if (!sessionContext?.session?.kernel) {
-      label = 'No Kernel';
+    if (sessionContext.kernelDisplayName === Private.NO_KERNEL) {
+      label = Private.NO_KERNEL;
     }
     const buttons = [
       Dialog.cancelButton({ label }),
@@ -1025,7 +1114,10 @@ export const sessionContextDialogs: ISessionContext.IDialogs = {
       return;
     }
     let model = result.value;
-    if (model === null && sessionContext?.session?.kernel) {
+    if (
+      model === null &&
+      sessionContext.kernelDisplayName !== Private.NO_KERNEL
+    ) {
       return sessionContext.shutdown();
     }
     if (model) {
@@ -1083,6 +1175,11 @@ export const sessionContextDialogs: ISessionContext.IDialogs = {
  * The namespace for module private data.
  */
 namespace Private {
+  /**
+   * The text to show for no kernel.
+   */
+  export const NO_KERNEL = 'No Kernel';
+
   /**
    * A widget that provides a kernel selection.
    */
@@ -1364,7 +1461,7 @@ namespace Private {
     let group = document.createElement('optgroup');
     group.label = 'Use No Kernel';
     let option = document.createElement('option');
-    option.text = 'No Kernel';
+    option.text = Private.NO_KERNEL;
     option.value = 'null';
     group.appendChild(option);
     return group;

packages/apputils/src/toolbar.tsx

@@ -752,7 +752,10 @@ namespace Private {
      */
     private _isBusy(status: ISessionContext.KernelDisplayStatus): boolean {
       return (
-        status === 'busy' || status === 'starting' || status === 'restarting'
+        status === 'busy' ||
+        status === 'starting' ||
+        status === 'restarting' ||
+        status === 'initializing'
       );
     }
   }

packages/statusbar/src/defaults/kernelStatus.tsx

@@ -19,12 +19,14 @@ import { JSONExt, JSONArray } from '@lumino/coreutils';
 function KernelStatusComponent(
   props: KernelStatusComponent.IProps
 ): React.ReactElement<KernelStatusComponent.IProps> {
+  let statusText = '';
+  if (props.status) {
+    statusText = ` | ${Text.titleCase(props.status)}`;
+  }
   return (
     <TextItem
       onClick={props.handleClick}
-      source={`${props.kernelName} | ${Text.titleCase(
-        props.status ?? 'undefined'
-      )}`}
+      source={`${props.kernelName}${statusText}`}
       title={`Change kernel for ${props.activityName}`}
     />
   );
@@ -147,7 +149,7 @@ export namespace KernelStatus {
       const oldState = this._getAllState();
       this._sessionContext = sessionContext;
       this._kernelStatus = sessionContext?.kernelDisplayStatus;
-      this._kernelName = sessionContext?.kernelDisplayName ?? 'No Kernel!';
+      this._kernelName = sessionContext?.kernelDisplayName ?? 'No Kernel';
       sessionContext?.statusChanged.connect(this._onKernelStatusChanged, this);
       sessionContext?.connectionStatusChanged.connect(
         this._onKernelStatusChanged,
@@ -173,17 +175,11 @@ export namespace KernelStatus {
       change: Session.ISessionConnection.IKernelChangedArgs
     ) => {
       const oldState = this._getAllState();
-      const { newValue } = change;
-      if (newValue !== null) {
-        // sync setting of status and display name
-        this._kernelStatus = this._sessionContext?.kernelDisplayStatus;
-        this._kernelName = _sessionContext.kernelDisplayName;
-        this._triggerChange(oldState, this._getAllState());
-      } else {
-        this._kernelStatus = '';
-        this._kernelName = 'No Kernel!';
-        this._triggerChange(oldState, this._getAllState());
-      }
+
+      // sync setting of status and display name
+      this._kernelStatus = this._sessionContext?.kernelDisplayStatus;
+      this._kernelName = _sessionContext.kernelDisplayName;
+      this._triggerChange(oldState, this._getAllState());
     };
 
     private _getAllState(): Private.State {

tests/test-apputils/src/sessioncontext.spec.ts

@@ -72,6 +72,7 @@ describe('@jupyterlab/apputils', () => {
 
     describe('#disposed', () => {
       it('should be emitted when the session context is disposed', async () => {
+        sessionContext.kernelPreference = { canStart: false };
         await sessionContext.initialize();
         let called = false;
         sessionContext.disposed.connect((sender, args) => {
@@ -235,7 +236,6 @@ describe('@jupyterlab/apputils', () => {
       it('should connect to an existing kernel', async () => {
         // Shut down and dispose the session so it can be re-instantiated.
         await sessionContext.shutdown();
-        sessionContext.dispose();
 
         const other = await sessionManager.startNew({
           name: '',
@@ -286,17 +286,21 @@ describe('@jupyterlab/apputils', () => {
         expect(sessionContext.kernelDisplayName).to.equal(spec!.display_name);
       });
 
-      it('should display "No Kernel!" when there is no kernel', async () => {
+      it('should display "No Kernel" when there is no kernel', async () => {
         sessionContext.kernelPreference = {
           canStart: false,
           shouldStart: false
         };
-        expect(sessionContext.kernelDisplayName).to.equal('No Kernel!');
+        expect(sessionContext.kernelDisplayName).to.equal('No Kernel');
       });
 
-      it('should display "Kernel" when it looks like we are starting a kernel', async () => {
-        sessionContext.kernelPreference = {};
-        expect(sessionContext.kernelDisplayName).to.equal('Kernel');
+      it('should display the pending kernel name when it looks like we are starting a kernel', async () => {
+        sessionContext.kernelPreference = {
+          autoStartDefault: true,
+          canStart: true,
+          shouldStart: true
+        };
+        expect(sessionContext.kernelDisplayName).to.equal('Echo Kernel');
       });
     });
 
@@ -323,10 +327,10 @@ describe('@jupyterlab/apputils', () => {
         expect(sessionContext.kernelDisplayStatus).to.be.equal('initializing');
       });
 
-      it('should be "" if there is no current kernel', async () => {
+      it('should be "idle" if there is no current kernel', async () => {
         await sessionContext.initialize();
         await sessionContext.shutdown();
-        expect(sessionContext.kernelDisplayStatus).to.be.equal('');
+        expect(sessionContext.kernelDisplayStatus).to.be.equal('idle');
       });
     });
 
@@ -352,18 +356,24 @@ describe('@jupyterlab/apputils', () => {
         sessionContext.dispose();
         const sessions = await SessionAPI.listRunning();
         expect(sessions.find(s => s.id === id)).to.be.ok;
+        await SessionAPI.shutdownSession(id);
       });
 
-      it('should shut down the session when shutdownOnDispose is true', async () => {
+      it('should shut down the session when shutdownOnDispose is true', async done => {
         sessionContext.kernelPreference = {
           ...sessionContext.kernelPreference,
           shutdownOnDispose: true
         };
         await sessionContext.initialize();
         const id = sessionContext.session!.id;
+        // Wait for the session to shut down.
+        sessionContext.sessionManager.runningChanged.connect((_, sessions) => {
+          if (!sessions.find(s => s.id === id)) {
+            done();
+            return;
+          }
+        });
         sessionContext.dispose();
-        const sessions = await SessionAPI.listRunning();
-        expect(sessions.find(s => s.id === id)).to.be.undefined;
       });
     });
 
@@ -378,6 +388,39 @@ describe('@jupyterlab/apputils', () => {
         expect(kernel.id).to.not.equal(id);
         expect(kernel.name).to.equal(name);
       });
+
+      it('should still work if called before fully initialized', async () => {
+        const initPromise = sessionContext.initialize(); // Start but don't finish init.
+        const name = 'echo';
+        const kernelPromise = sessionContext.changeKernel({ name });
+
+        let lastKernel = null;
+        sessionContext.kernelChanged.connect(() => {
+          lastKernel = sessionContext.session?.kernel;
+        });
+        const results = await Promise.all([kernelPromise, initPromise]);
+        const kernel = results[0];
+        const shouldSelect = results[1];
+        expect(shouldSelect).to.equal(false);
+        expect(lastKernel).to.equal(kernel);
+      });
+
+      it('should handle multiple requests', async () => {
+        await sessionContext.initialize();
+        const name = 'echo';
+        const kernelPromise0 = sessionContext.changeKernel({ name });
+        // The last launched kernel should win.
+        const kernelPromise1 = sessionContext.changeKernel({ name });
+
+        let lastKernel = null;
+        sessionContext.kernelChanged.connect(() => {
+          lastKernel = sessionContext.session?.kernel;
+        });
+        const results = await Promise.all([kernelPromise0, kernelPromise1]);
+        // We can't know which of the two was launched first, so the result
+        // could be either, just make sure it isn't the original kernel.
+        expect(lastKernel).to.be.oneOf([results[0], results[1]]);
+      });
     });
 
     describe('#shutdown', () => {
@@ -387,13 +430,17 @@ describe('@jupyterlab/apputils', () => {
         await sessionContext.shutdown();
         expect(sessionContext.session?.kernel).to.not.be.ok;
       });
-    });
 
-    describe('.getDefaultKernel()', () => {
-      beforeEach(() => {
-        sessionContext.dispose();
+      it('should handle a shutdown during startup', async () => {
+        const initPromise = sessionContext.initialize(); // Start but don't finish init.
+        const shutdownPromise = sessionContext.shutdown();
+        const results = await Promise.all([initPromise, shutdownPromise]);
+        expect(results[0]).to.equal(false);
+        expect(sessionContext.session).to.equal(null);
       });
+    });
 
+    describe('.getDefaultKernel()', () => {
       it('should return null if no options are given', () => {
         expect(
           SessionContext.getDefaultKernel({
@@ -467,28 +514,28 @@ describe('@jupyterlab/apputils', () => {
       describe('#selectKernel()', () => {
         it('should select a kernel for the session', async () => {
           await sessionContext.initialize();
-          const session = sessionContext?.session;
 
-          const { id, name } = session!.kernel!;
+          const { id, name } = sessionContext?.session!.kernel!;
           const accept = acceptDialog();
 
           await sessionContextDialogs.selectKernel(sessionContext);
           await accept;
 
+          const session = sessionContext?.session;
           expect(session!.kernel!.id).to.not.equal(id);
           expect(session!.kernel!.name).to.equal(name);
         });
 
         it('should keep the existing kernel if dismissed', async () => {
           await sessionContext.initialize();
-          const session = sessionContext!.session;
 
-          const { id, name } = session!.kernel!;
+          const { id, name } = sessionContext!.session!.kernel!;
           const dismiss = dismissDialog();
 
           await sessionContextDialogs.selectKernel(sessionContext);
           await dismiss;
 
+          const session = sessionContext.session;
           expect(session!.kernel!.id).to.equal(id);
           expect(session!.kernel!.name).to.equal(name);
         });