Delete unnecessary awaits for kernel info replies.

This also (finally!) resolves the testing issues with kernel info messages being outstanding when the kernel is disposed.

packages/services/src/kernel/default.ts

@@ -413,10 +413,10 @@ export class DefaultKernel implements Kernel.IKernel {
     // Send if the ws allows it, otherwise buffer the message.
     if (this.connectionStatus === 'connected') {
       this._ws.send(serialize.serialize(msg));
-      console.log(`SENT WS message to ${this.id}`, msg);
+      // console.log(`SENT WS message to ${this.id}`, msg);
     } else if (pending) {
       this._pendingMessages.push(msg);
-      console.log(`PENDING WS message to ${this.id}`, msg);
+      // console.log(`PENDING WS message to ${this.id}`, msg);
     } else {
       throw new Error('Could not send message');
     }
@@ -467,7 +467,7 @@ export class DefaultKernel implements Kernel.IKernel {
    * interface.
    */
   async handleRestart(): Promise<void> {
-    await this._clearState();
+    this._clearState();
     this._updateStatus('restarting');
 
     // Kick off an async kernel request to eventually reset the kernel status.
@@ -528,11 +528,11 @@ export class DefaultKernel implements Kernel.IKernel {
     if (this.status === 'dead') {
       this._updateConnectionStatus('disconnected');
       this._clearSocket();
-      await this._clearState();
+      this._clearState();
       return;
     }
     await Private.shutdownKernel(this.id, this.serverSettings);
-    await this._clearState();
+    this._clearState();
     this._updateConnectionStatus('disconnected');
     this._clearSocket();
   }
@@ -554,10 +554,20 @@ export class DefaultKernel implements Kernel.IKernel {
       session: this._clientId,
       content: {}
     });
-    let reply = (await Private.handleShellMessage(
-      this,
-      msg
-    )) as KernelMessage.IInfoReplyMsg;
+    let reply: KernelMessage.IInfoReplyMsg;
+    try {
+      reply = (await Private.handleShellMessage(
+        this,
+        msg
+      )) as KernelMessage.IInfoReplyMsg;
+    } catch (e) {
+      // If we rejected because the future was disposed, ignore and return.
+      if (this.isDisposed) {
+        return;
+      } else {
+        throw e;
+      }
+    }
     if (this.isDisposed) {
       throw new Error('Disposed kernel');
     }
@@ -1063,11 +1073,9 @@ export class DefaultKernel implements Kernel.IKernel {
   /**
    * Clear the internal state.
    */
-  private async _clearState(): Promise<void> {
+  private _clearState(): void {
     this._pendingMessages = [];
-    const futuresResolved: Promise<void>[] = [];
     this._futures.forEach(future => {
-      futuresResolved.push(future.done.then(this._noOp, this._noOp));
       future.dispose();
     });
     this._comms.forEach(comm => {
@@ -1085,8 +1093,6 @@ export class DefaultKernel implements Kernel.IKernel {
     this._comms = new Map<string, Kernel.IComm>();
     this._displayIdToParentIds.clear();
     this._msgIdToDisplayIds.clear();
-
-    await Promise.all(futuresResolved);
   }
 
   /**
@@ -1912,10 +1918,9 @@ namespace Private {
   /**
    * Send a kernel message to the kernel and resolve the reply message.
    */
-  export async function handleShellMessage(
-    kernel: Kernel.IKernel,
-    msg: KernelMessage.IShellMessage
-  ): Promise<KernelMessage.IShellMessage> {
+  export async function handleShellMessage<
+    T extends KernelMessage.ShellMessageType
+  >(kernel: Kernel.IKernel, msg: KernelMessage.IShellMessage<T>) {
     let future = kernel.sendShellMessage(msg, true);
     return future.done;
   }

packages/services/src/kernel/future.ts

@@ -167,15 +167,6 @@ export abstract class KernelFutureHandler<
     this._reply = Private.noOp;
     this._hooks = null;
     if (!this._testFlag(Private.KernelFutureFlag.IsDone)) {
-      // Reject the `done` promise, but catch its error here in case no one else
-      // is waiting for the promise to resolve. This prevents the error from
-      // being displayed in the console, but does not prevent it from being
-      // caught by a client who is waiting for it.
-      this._done.reject(new Error('Canceled'));
-      this._done.promise.catch(() => {
-        /* no-op */
-      });
-
       // TODO: Uncomment the following logging code, and check for any tests that trigger it.
       // let status = [];
       // if (!this._testFlag(Private.KernelFutureFlag.GotIdle)) {
@@ -184,7 +175,23 @@ export abstract class KernelFutureHandler<
       // if (!this._testFlag(Private.KernelFutureFlag.GotReply)) {
       //   status.push('reply');
       // }
-      // console.warn(`*************** DISPOSED BEFORE DONE: K${this._kernel.id.slice(0, 6)} M${this._msg.header.msg_id.slice(0, 6)} missing ${status.join(' ')}`);
+      // console.warn(
+      //   `*************** DISPOSED BEFORE DONE: K${this._kernel.id.slice(
+      //     0,
+      //     6
+      //   )} M${this._msg.header.msg_id.slice(0, 6)} missing ${status.join(' ')}`
+      // );
+
+      // Reject the `done` promise, but catch its error here in case no one else
+      // is waiting for the promise to resolve. This prevents the error from
+      // being displayed in the console, but does not prevent it from being
+      // caught by a client who is waiting for it.
+      this._done.promise.catch(() => {
+        /* no-op */
+      });
+      this._done.reject(
+        `Canceled future for ${this.msg.header.msg_type} message before replies were done`
+      );
     }
     super.dispose();
   }

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

@@ -321,14 +321,11 @@ describe('@jupyterlab/apputils', () => {
           find: (_, args) => args === 'restarting'
         });
         await session.initialize();
-        await session.kernel.info;
         const restart = session.restart();
 
         await acceptDialog();
         expect(await restart).to.equal(true);
         await emission;
-        // Wait for the restarted kernel to be ready
-        await session.kernel.info;
       });
 
       it('should not restart if the user rejects the dialog', async () => {
@@ -390,7 +387,6 @@ describe('@jupyterlab/apputils', () => {
           }
         });
         await session.initialize();
-        await session.kernel.info;
 
         const restart = ClientSession.restartKernel(session.kernel);
 
@@ -408,7 +404,6 @@ describe('@jupyterlab/apputils', () => {
           }
         });
         await session.initialize();
-        await session.kernel.info;
 
         const restart = ClientSession.restartKernel(session.kernel);
 

tests/test-services/src/kernel/ikernel.spec.ts

@@ -26,7 +26,6 @@ describe('Kernel.IKernel', () => {
 
   beforeEach(async () => {
     defaultKernel = await Kernel.startNew();
-    await defaultKernel.info;
   });
 
   afterEach(async () => {
@@ -77,7 +76,6 @@ describe('Kernel.IKernel', () => {
     it('should be emitted regardless of the sender', async () => {
       const tester = new KernelTester();
       const kernel = await tester.start();
-      await kernel.info;
       const msgId = UUID.uuid4();
       const emission = testEmission(kernel.iopubMessage, {
         find: (k, msg) => msg.header.msg_id === msgId
@@ -305,6 +303,10 @@ describe('Kernel.IKernel', () => {
   });
 
   describe('#status', () => {
+    beforeEach(async () => {
+      await defaultKernel.info;
+    });
+
     it('should get an idle status', async () => {
       const emission = testEmission(defaultKernel.statusChanged, {
         find: () => defaultKernel.status === 'idle'
@@ -317,6 +319,8 @@ describe('Kernel.IKernel', () => {
       const emission = testEmission(defaultKernel.statusChanged, {
         find: () => defaultKernel.status === 'restarting'
       });
+      // TODO: should restart cancel all existing futures and catch the
+      // errors?
       await defaultKernel.restart();
       await emission;
     });
@@ -353,27 +357,6 @@ describe('Kernel.IKernel', () => {
       await dead;
       tester.dispose();
     });
-
-    it('should not emit an invalid status', async () => {
-      const tester = new KernelTester();
-      const kernel = await tester.start();
-      await kernel.info;
-      const emission = testEmission(kernel.statusChanged, {
-        test: (k, status) => {
-          expect(status).to.equal('busy');
-          expect(kernel.status).to.equal('busy');
-        }
-      });
-
-      // This invalid status is not emitted.
-      tester.sendStatus(UUID.uuid4(), 'invalid-status' as Kernel.Status);
-
-      // This valid status is emitted.
-      tester.sendStatus(UUID.uuid4(), 'busy');
-
-      await emission;
-      tester.dispose();
-    });
   });
 
   describe('#info', () => {
@@ -392,14 +375,14 @@ describe('Kernel.IKernel', () => {
   });
 
   describe('#isDisposed', () => {
-    it('should be true after we dispose of the kernel', () => {
+    it('should be true after we dispose of the kernel', async () => {
       const kernel = Kernel.connectTo(defaultKernel.model);
       expect(kernel.isDisposed).to.equal(false);
       kernel.dispose();
       expect(kernel.isDisposed).to.equal(true);
     });
 
-    it('should be safe to call multiple times', () => {
+    it('should be safe to call multiple times', async () => {
       const kernel = Kernel.connectTo(defaultKernel.model);
       expect(kernel.isDisposed).to.equal(false);
       expect(kernel.isDisposed).to.equal(false);
@@ -410,7 +393,7 @@ describe('Kernel.IKernel', () => {
   });
 
   describe('#dispose()', () => {
-    it('should dispose of the resources held by the kernel', () => {
+    it('should dispose of the resources held by the kernel', async () => {
       const kernel = Kernel.connectTo(defaultKernel.model);
       const future = kernel.requestExecute({ code: 'foo' });
       expect(future.isDisposed).to.equal(false);
@@ -418,7 +401,7 @@ describe('Kernel.IKernel', () => {
       expect(future.isDisposed).to.equal(true);
     });
 
-    it('should be safe to call twice', () => {
+    it('should be safe to call twice', async () => {
       const kernel = Kernel.connectTo(defaultKernel.model);
       const future = kernel.requestExecute({ code: 'foo' });
       expect(future.isDisposed).to.equal(false);
@@ -573,7 +556,6 @@ describe('Kernel.IKernel', () => {
   describe('#interrupt()', () => {
     it('should interrupt and resolve with a valid server response', async () => {
       const kernel = await Kernel.startNew();
-      await kernel.info;
       await kernel.interrupt();
       await kernel.shutdown();
     });
@@ -609,9 +591,12 @@ describe('Kernel.IKernel', () => {
   });
 
   describe('#restart()', () => {
+    beforeEach(async () => {
+      await defaultKernel.info;
+    });
+
     it('should restart and resolve with a valid server response', async () => {
       await defaultKernel.restart();
-      await defaultKernel.info;
     });
 
     it('should fail if the kernel does not restart', async () => {
@@ -621,10 +606,10 @@ describe('Kernel.IKernel', () => {
     });
 
     it('should throw an error for an invalid response', async () => {
-      const kernel = defaultKernel;
-      handleRequest(kernel, 205, { id: kernel.id, name: kernel.name });
+      const { id, name } = defaultKernel;
+      handleRequest(defaultKernel, 205, { id, name });
       await expectFailure(
-        kernel.restart(),
+        defaultKernel.restart(),
         'Invalid response: 205 Reset Content'
       );
     });
@@ -642,11 +627,9 @@ describe('Kernel.IKernel', () => {
     });
 
     it('should dispose of existing comm and future objects', async () => {
-      const kernel = defaultKernel;
-      const comm = kernel.connectToComm('test');
-      const future = kernel.requestExecute({ code: 'foo' });
-      await kernel.restart();
-      await kernel.info;
+      const comm = defaultKernel.connectToComm('test');
+      const future = defaultKernel.requestExecute({ code: 'foo' });
+      await defaultKernel.restart();
       expect(future.isDisposed).to.equal(true);
       expect(comm.isDisposed).to.equal(true);
     });
@@ -658,6 +641,7 @@ describe('Kernel.IKernel', () => {
     });
 
     it('should emit `"reconnecting"`, then `"connected"` status', async () => {
+      await defaultKernel.info;
       let connectedEmission: Promise<void>;
       const emission = testEmission(defaultKernel.statusChanged, {
         find: () => defaultKernel.status === 'unknown',
@@ -677,7 +661,6 @@ describe('Kernel.IKernel', () => {
   describe('#shutdown()', () => {
     it('should shut down and resolve with a valid server response', async () => {
       const kernel = await Kernel.startNew();
-      await kernel.info;
       await kernel.shutdown();
     });
 
@@ -1319,6 +1302,7 @@ describe('Kernel.IKernel', () => {
 
       const tester = new KernelTester();
       const kernel = await tester.start();
+      await kernel.info;
       const future = kernel.requestExecute(options, false);
 
       // The list of emissions from the anyMessage signal.

tests/test-services/src/kernel/kernel.spec.ts

@@ -28,10 +28,6 @@ describe('kernel', () => {
 
   beforeAll(async () => {
     defaultKernel = await Kernel.startNew();
-    await defaultKernel.info;
-    // Start another kernel.
-    let k = await Kernel.startNew();
-    await k.info;
   });
 
   afterEach(() => {
@@ -82,7 +78,6 @@ describe('kernel', () => {
     it('should create an Kernel.IKernel object', async () => {
       const kernel = await Kernel.startNew({});
       expect(kernel.status).to.equal('unknown');
-      await kernel.info;
       await kernel.shutdown();
     });
 
@@ -90,7 +85,6 @@ describe('kernel', () => {
       const serverSettings = makeSettings();
       const kernel = await Kernel.startNew({ serverSettings });
       expect(kernel.status).to.equal('unknown');
-      await kernel.info;
       await kernel.shutdown();
     });
 
@@ -143,7 +137,6 @@ describe('kernel', () => {
     it('should auto-reconnect on websocket error', async () => {
       tester = new KernelTester();
       const kernel = await tester.start();
-      await kernel.info;
 
       const emission = testEmission(kernel.connectionStatusChanged, {
         find: (k, status) => status === 'connecting'
@@ -158,7 +151,6 @@ describe('kernel', () => {
       const id = defaultKernel.id;
       const kernel = Kernel.connectTo(defaultKernel.model);
       expect(kernel.id).to.equal(id);
-      await kernel.info;
       kernel.dispose();
     });
 
@@ -167,7 +159,6 @@ describe('kernel', () => {
       const serverSettings = makeSettings();
       const kernel = Kernel.connectTo(defaultKernel.model, serverSettings);
       expect(kernel.id).to.equal(id);
-      await kernel.info;
       kernel.dispose();
     });
 
@@ -176,8 +167,6 @@ describe('kernel', () => {
       expect(kernel.handleComms).to.be.true;
       const kernel2 = Kernel.connectTo(kernel.model);
       expect(kernel2.handleComms).to.be.false;
-      await kernel.info;
-      await kernel2.info;
       kernel.dispose();
       kernel2.dispose();
     });
@@ -185,10 +174,8 @@ describe('kernel', () => {
     it('should turn on comm handling in the new connection if it was disabled in all other connections', async () => {
       const kernel = await Kernel.startNew({ handleComms: false });
       expect(kernel.handleComms).to.be.false;
-      const kernel2 = Kernel.connectTo(defaultKernel.model);
+      const kernel2 = Kernel.connectTo(kernel.model);
       expect(kernel2.handleComms).to.be.true;
-      await kernel.info;
-      await kernel2.info;
       kernel.dispose();
       kernel2.dispose();
     });
@@ -197,7 +184,6 @@ describe('kernel', () => {
   describe('Kernel.shutdown()', () => {
     it('should shut down a kernel by id', async () => {
       const k = await Kernel.startNew();
-      await k.info;
       await Kernel.shutdown(k.id);
     });
 

tests/test-services/src/kernel/manager.spec.ts

@@ -213,7 +213,6 @@ describe('kernel/manager', () => {
     describe('shutdown()', () => {
       it('should shut down a kernel by id', async () => {
         const kernel = await manager.startNew();
-        await kernel.info;
         await manager.shutdown(kernel.id);
         expect(kernel.isDisposed).to.equal(true);
       });

tests/test-services/src/session/isession.spec.ts

@@ -24,7 +24,6 @@ init();
  */
 async function startNew(): Promise<Session.ISession> {
   const session = await Session.startNew({ path: UUID.uuid4() });
-  await session.kernel.info;
   return session;
 }
 
@@ -34,18 +33,15 @@ describe('session', () => {
 
   beforeAll(async () => {
     defaultSession = await startNew();
-    await defaultSession.kernel.info;
   });
 
   afterEach(async () => {
     if (session && !session.isDisposed) {
-      await session.kernel.info;
       await session.shutdown();
     }
   });
 
   afterAll(async () => {
-    await defaultSession.kernel.info;
     await defaultSession.shutdown();
   });
 
@@ -54,7 +50,6 @@ describe('session', () => {
       it('should emit when the session is disposed', async () => {
         let called = false;
         session = await startNew();
-        await session.kernel.info;
         session.disposed.connect(() => {
           called = true;
         });
@@ -74,7 +69,6 @@ describe('session', () => {
         }, object);
         const previous = defaultSession.kernel;
         await defaultSession.changeKernel({ name: previous.name });
-        await defaultSession.kernel.info;
         expect(previous).to.not.equal(defaultSession.kernel);
         expect(called).to.deep.equal({
           oldValue: previous,
@@ -114,7 +108,6 @@ describe('session', () => {
       it('should be emitted for an unhandled message', async () => {
         const tester = new SessionTester();
         const session = await tester.startSession();
-        await session.kernel.info;
         const msgId = UUID.uuid4();
         const emission = testEmission(session.unhandledMessage, {
           find: (k, msg) => msg.header.msg_id === msgId
@@ -190,12 +183,6 @@ describe('session', () => {
       });
     });
 
-    describe('#kernel', () => {
-      it('should be a delegate to the kernel status', () => {
-        expect(defaultSession.status).to.equal(defaultSession.kernel.status);
-      });
-    });
-
     describe('#serverSettings', () => {
       it('should be the serverSettings', () => {
         expect(defaultSession.serverSettings.baseUrl).to.equal(
@@ -205,15 +192,15 @@ describe('session', () => {
     });
 
     describe('#isDisposed', () => {
-      it('should be true after we dispose of the session', () => {
-        const session = Session.connectTo(defaultSession.model);
+      it('should be true after we dispose of the session', async () => {   
+        const session = await startNew();
         expect(session.isDisposed).to.equal(false);
         session.dispose();
         expect(session.isDisposed).to.equal(true);
       });
 
-      it('should be safe to call multiple times', () => {
-        const session = Session.connectTo(defaultSession.model);
+      it('should be safe to call multiple times', async () => {
+        const session = await startNew();
         expect(session.isDisposed).to.equal(false);
         expect(session.isDisposed).to.equal(false);
         session.dispose();
@@ -223,22 +210,22 @@ describe('session', () => {
     });
 
     describe('#dispose()', () => {
-      it('should dispose of the resources held by the session', () => {
-        const session = Session.connectTo(defaultSession.model);
+      it('should dispose of the resources held by the session', async () => {
+        const session = await startNew();
         session.dispose();
         expect(session.isDisposed).to.equal(true);
       });
 
-      it('should be safe to call twice', () => {
-        const session = Session.connectTo(defaultSession.model);
+      it('should be safe to call twice', async () => {
+        const session = await startNew();
         session.dispose();
         expect(session.isDisposed).to.equal(true);
         session.dispose();
         expect(session.isDisposed).to.equal(true);
       });
 
-      it('should be safe to call if the kernel is disposed', () => {
-        const session = Session.connectTo(defaultSession.model);
+      it('should be safe to call if the kernel is disposed', async () => {
+        const session = await startNew();
         session.kernel.dispose();
         session.dispose();
         expect(session.isDisposed).to.equal(true);
@@ -277,9 +264,11 @@ describe('session', () => {
 
     describe('#setType()', () => {
       it('should set the type of the session', async () => {
+        const session = await startNew();
         const type = UUID.uuid4();
-        await defaultSession.setType(type);
-        expect(defaultSession.type).to.equal(type);
+        await session.setType(type);
+        expect(session.type).to.equal(type);
+        await session.shutdown();
       });
 
       it('should fail for improper response status', async () => {
@@ -341,7 +330,6 @@ describe('session', () => {
         const previous = session.kernel;
         await previous.info;
         await session.changeKernel({ name: previous.name });
-        await session.kernel.info;
         expect(session.kernel.name).to.equal(previous.name);
         expect(session.kernel.id).to.not.equal(previous.id);
         expect(session.kernel).to.not.equal(previous);
@@ -353,9 +341,7 @@ describe('session', () => {
         const previous = session.kernel;
         await previous.info;
         const kernel = await Kernel.startNew();
-        await kernel.info;
         await session.changeKernel({ id: kernel.id });
-        await session.kernel.info;
         expect(session.kernel.id).to.equal(kernel.id);
         expect(session.kernel).to.not.equal(previous);
         expect(session.kernel).to.not.equal(kernel);
@@ -370,7 +356,6 @@ describe('session', () => {
         const model = { ...session.model, path: 'foo.ipynb' };
         handleRequest(session, 200, model);
         await session.changeKernel({ name: previous.name });
-        await session.kernel.info;
         expect(session.kernel.name).to.equal(previous.name);
         expect(session.path).to.equal(model.path);
         previous.dispose();

tests/test-services/src/session/manager.spec.ts

@@ -35,7 +35,6 @@ class TestManager extends SessionManager {
  */
 async function startNew(manager: SessionManager): Promise<Session.ISession> {
   const session = await manager.startNew({ path: UUID.uuid4() });
-  await session.kernel.info;
   return session;
 }
 
@@ -45,7 +44,6 @@ describe('session/manager', () => {
 
   beforeAll(async () => {
     session = await Session.startNew({ path: UUID.uuid4() });
-    await session.kernel.info;
   });
 
   beforeEach(() => {
@@ -142,7 +140,6 @@ describe('session/manager', () => {
       it('should be emitted when a session is shut down', async () => {
         let called = false;
         const s = await startNew(manager);
-        await s.kernel.info;
         manager.runningChanged.connect(() => {
           called = true;
         });
@@ -198,7 +195,6 @@ describe('session/manager', () => {
     describe('#startNew()', () => {
       it('should start a session', async () => {
         const session = await manager.startNew({ path: UUID.uuid4() });
-        await session.kernel.info;
         expect(session.id).to.be.ok;
         return session.shutdown();
       });
@@ -208,8 +204,7 @@ describe('session/manager', () => {
         manager.runningChanged.connect(() => {
           called = true;
         });
-        const session = await manager.startNew({ path: UUID.uuid4() });
-        await session.kernel.info;
+        await manager.startNew({ path: UUID.uuid4() });
         expect(called).to.equal(true);
       });
     });
@@ -241,7 +236,6 @@ describe('session/manager', () => {
     describe('shutdown()', () => {
       it('should shut down a session by id', async () => {
         const temp = await startNew(manager);
-        await temp.kernel.info;
         await manager.shutdown(temp.id);
         expect(temp.isDisposed).to.equal(true);
       });
@@ -249,7 +243,6 @@ describe('session/manager', () => {
       it('should emit a runningChanged signal', async () => {
         let called = false;
         const session = await startNew(manager);
-        await session.kernel.info;
         manager.runningChanged.connect((sender, sessions) => {
           // Make sure the sessions list does not have our shutdown session in it.
           if (!sessions.find(s => s.id === session.id)) {

tests/test-services/src/session/session.spec.ts

@@ -50,18 +50,15 @@ describe('session', () => {
 
   beforeAll(async () => {
     defaultSession = await startNew();
-    await defaultSession.kernel.info;
   });
 
   afterEach(async () => {
     if (session && !session.isDisposed) {
-      await session.kernel.info;
       await session.shutdown();
     }
   });
 
   afterAll(async () => {
-    await defaultSession.kernel.info;
     await defaultSession.shutdown();
   });
 
@@ -190,7 +187,6 @@ describe('session', () => {
   describe('Session.shutdown()', () => {
     it('should shut down a kernel by id', async () => {
       session = await startNew();
-      await session.kernel.info;
       await Session.shutdown(session.id);
     });
 

tests/test-services/src/utils.ts

@@ -454,7 +454,6 @@ export class KernelTester extends SocketTester {
     const serverSettings = this.serverSettings;
     this._kernel = await Kernel.startNew({ serverSettings });
     await this.ready;
-    await this._kernel.info;
     return this._kernel;
   }
 
@@ -561,7 +560,6 @@ export class SessionTester extends SocketTester {
       serverSettings
     });
     await this.ready;
-    await this._session.kernel.info;
     return this._session;
   }