Merge pull request #6339 from blink1073/terminal-zombies

Handle disconnect messages from the terminal

examples/terminal/src/index.ts

@@ -3,6 +3,7 @@
 
 import 'es6-promise/auto'; // polyfill Promise on IE
 import '@jupyterlab/application/style/index.css';
+import '@jupyterlab/terminal/style/index.css';
 import '@jupyterlab/theme-light-extension/style/index.css';
 import '../index.css';
 
@@ -12,22 +13,8 @@ import { TerminalSession } from '@jupyterlab/services';
 
 import { Terminal } from '@jupyterlab/terminal';
 
-function main(): void {
-  let term1 = new Terminal({ theme: 'light' });
-  let term2 = new Terminal({ theme: 'dark' });
-
-  void TerminalSession.startNew().then(session => {
-    term1.session = session;
-  });
-  void TerminalSession.startNew().then(session => {
-    term2.session = session;
-  });
-
-  term1.title.closable = true;
-  term2.title.closable = true;
+async function main(): Promise<void> {
   let dock = new DockPanel();
-  dock.addWidget(term1);
-  dock.addWidget(term2, { mode: 'tab-before' });
   dock.id = 'main';
 
   // Attach the widget to the dom.
@@ -37,6 +24,16 @@ function main(): void {
   window.addEventListener('resize', () => {
     dock.fit();
   });
+
+  const s1 = await TerminalSession.startNew();
+  const term1 = new Terminal(s1, { theme: 'light' });
+  term1.title.closable = true;
+  dock.addWidget(term1);
+
+  const s2 = await TerminalSession.startNew();
+  const term2 = new Terminal(s2, { theme: 'dark' });
+  term2.title.closable = true;
+  dock.addWidget(term2, { mode: 'tab-before' });
 }
 
 window.addEventListener('load', main);

packages/application/style/index.css

@@ -5,6 +5,7 @@
 
 @import url('~font-awesome/css/font-awesome.min.css');
 @import url('~@phosphor/widgets/style/index.css');
+@import url('~@jupyterlab/apputils/style/index.css');
 
 .p-Widget.p-mod-hidden {
   display: none !important;

packages/services/src/terminal/default.ts

@@ -197,6 +197,11 @@ export class DefaultTerminalSession implements TerminalSession.ISession {
 
         const data = JSON.parse(event.data) as JSONPrimitive[];
 
+        // Handle a disconnect message.
+        if (data[0] === 'disconnect') {
+          this._disconnected = true;
+        }
+
         if (this._reconnectAttempt > 0) {
           // After reconnection, ignore all messages until a 'setup' message.
           if (data[0] === 'setup') {
@@ -214,6 +219,7 @@ export class DefaultTerminalSession implements TerminalSession.ISession {
       socket.onopen = (event: MessageEvent) => {
         if (!this._isDisposed) {
           this._isReady = true;
+          this._disconnected = false;
           resolve(undefined);
         }
       };
@@ -226,13 +232,16 @@ export class DefaultTerminalSession implements TerminalSession.ISession {
 
       socket.onclose = (event: CloseEvent) => {
         console.warn(`Terminal websocket closed: ${event.code}`);
+        if (this._disconnected) {
+          this.dispose();
+        }
         this._reconnectSocket();
       };
     });
   }
 
   private _reconnectSocket(): void {
-    if (this._isDisposed || !this._ws) {
+    if (this._isDisposed || !this._ws || this._disconnected) {
       return;
     }
 
@@ -277,6 +286,7 @@ export class DefaultTerminalSession implements TerminalSession.ISession {
   };
   private _reconnectLimit = 7;
   private _reconnectAttempt = 0;
+  private _disconnected = false;
 }
 
 /**
@@ -433,13 +443,11 @@ export namespace DefaultTerminalSession {
       if (response.status === 404) {
         return response.json().then(data => {
           console.warn(data['message']);
-          Private.killTerminal(url);
         });
       }
       if (response.status !== 204) {
         throw new ServerConnection.ResponseError(response);
       }
-      Private.killTerminal(url);
     });
   }
 
@@ -488,15 +496,4 @@ namespace Private {
   export function getServiceUrl(baseUrl: string): string {
     return URLExt.join(baseUrl, TERMINAL_SERVICE_URL);
   }
-
-  /**
-   * Kill a terminal by url.
-   */
-  export function killTerminal(url: string): void {
-    // Update the local data store.
-    if (Private.running[url]) {
-      let session = Private.running[url];
-      session.dispose();
-    }
-  }
 }

packages/terminal-extension/src/index.ts

@@ -107,7 +107,7 @@ function activate(
     restorer.restore(tracker, {
       command: CommandIDs.createNew,
       args: widget => ({ name: widget.content.session.name }),
-      name: widget => widget.content.session && widget.content.session.name
+      name: widget => widget.content.session.name
     });
   }
 
@@ -270,27 +270,22 @@ export function addCommands(
       }
 
       const name = args['name'] as string;
-      const term = new Terminal();
+
+      const session = await (name
+        ? serviceManager.terminals
+            .connectTo(name)
+            .catch(() => serviceManager.terminals.startNew())
+        : serviceManager.terminals.startNew());
+
+      const term = new Terminal(session);
 
       term.title.icon = TERMINAL_ICON_CLASS;
       term.title.label = '...';
+
       let main = new MainAreaWidget({ content: term });
       app.shell.add(main);
-
-      try {
-        term.session = await (name
-          ? serviceManager.terminals
-              .connectTo(name)
-              .catch(() => serviceManager.terminals.startNew())
-          : serviceManager.terminals.startNew());
-
-        void tracker.add(main);
-        app.shell.activateById(main.id);
-
-        return main;
-      } catch {
-        term.dispose();
-      }
+      void tracker.add(main);
+      app.shell.activateById(main.id);
     }
   });
 
@@ -300,7 +295,7 @@ export function addCommands(
       // Check for a running terminal with the given name.
       const widget = tracker.find(value => {
         let content = value.content;
-        return (content.session && content.session.name === name) || false;
+        return content.session.name === name || false;
       });
       if (widget) {
         app.shell.activateById(widget.id);

packages/terminal/src/constants.ts

@@ -35,7 +35,7 @@ export namespace ITerminal {
     /**
      * The terminal session associated with the widget.
      */
-    session: TerminalSession.ISession | null;
+    session: TerminalSession.ISession;
 
     /**
      * Get a config option for the terminal.

packages/terminal/src/widget.ts

@@ -30,11 +30,18 @@ export class Terminal extends Widget implements ITerminal.ITerminal {
   /**
    * Construct a new terminal widget.
    *
+   * @param session - The terminal session object.
+   *
    * @param options - The terminal configuration options.
    */
-  constructor(options: Partial<ITerminal.IOptions> = {}) {
+  constructor(
+    session: TerminalSession.ISession,
+    options: Partial<ITerminal.IOptions> = {}
+  ) {
     super();
 
+    this.session = session;
+
     // Initialize settings.
     this._options = { ...ITerminal.defaultOptions, ...options };
 
@@ -49,31 +56,19 @@ export class Terminal extends Widget implements ITerminal.ITerminal {
 
     this.id = `jp-Terminal-${Private.id++}`;
     this.title.label = 'Terminal';
-  }
 
-  /**
-   * The terminal session associated with the widget.
-   */
-  get session(): TerminalSession.ISession | null {
-    return this._session;
-  }
-  set session(value: TerminalSession.ISession | null) {
-    if (this._session && !this._session.isDisposed) {
-      this._session.messageReceived.disconnect(this._onMessage, this);
-    }
-    this._session = value || null;
-    if (!value) {
-      return;
-    }
-    void value.ready.then(() => {
-      if (this.isDisposed || value !== this._session) {
+    session.messageReceived.connect(this._onMessage, this);
+    session.terminated.connect(this.dispose, this);
+
+    void session.ready.then(() => {
+      if (this.isDisposed) {
         return;
       }
-      value.messageReceived.connect(this._onMessage, this);
-      this.title.label = `Terminal ${value.name}`;
+
+      this.title.label = `Terminal ${session.name}`;
       this._setSessionSize();
       if (this._options.initialCommand) {
-        this._session.send({
+        this.session.send({
           type: 'stdin',
           content: [this._options.initialCommand + '\r']
         });
@@ -81,6 +76,11 @@ export class Terminal extends Widget implements ITerminal.ITerminal {
     });
   }
 
+  /**
+   * The terminal session associated with the widget.
+   */
+  readonly session: TerminalSession.ISession;
+
   /**
    * Get a config option for the terminal.
    */
@@ -128,14 +128,13 @@ export class Terminal extends Widget implements ITerminal.ITerminal {
    * Dispose of the resources held by the terminal widget.
    */
   dispose(): void {
-    if (this._session) {
+    if (!this.session.isDisposed) {
       if (this.getOption('shutdownOnClose')) {
-        this._session.shutdown().catch(reason => {
+        this.session.shutdown().catch(reason => {
           console.error(`Terminal not shut down: ${reason}`);
         });
       }
     }
-    this._session = null;
     this._term.dispose();
     super.dispose();
   }
@@ -147,8 +146,8 @@ export class Terminal extends Widget implements ITerminal.ITerminal {
    * Failure to reconnect to the session should be caught appropriately
    */
   async refresh(): Promise<void> {
-    if (this._session) {
-      await this._session.reconnect();
+    if (!this.isDisposed) {
+      await this.session.reconnect();
       this._term.clear();
     }
   }
@@ -236,12 +235,13 @@ export class Terminal extends Widget implements ITerminal.ITerminal {
    */
   private _initializeTerm(): void {
     this._term.on('data', (data: string) => {
-      if (this._session) {
-        this._session.send({
-          type: 'stdin',
-          content: [data]
-        });
+      if (this.isDisposed) {
+        return;
       }
+      this.session.send({
+        type: 'stdin',
+        content: [data]
+      });
     });
 
     this._term.on('title', (title: string) => {
@@ -295,14 +295,13 @@ export class Terminal extends Widget implements ITerminal.ITerminal {
       this._offsetHeight,
       this._offsetWidth
     ];
-    if (this._session) {
-      this._session.send({ type: 'set_size', content });
+    if (!this.isDisposed) {
+      this.session.send({ type: 'set_size', content });
     }
   }
 
   private _term: Xterm;
   private _needsResize = true;
-  private _session: TerminalSession.ISession | null = null;
   private _termOpened = false;
   private _offsetWidth = -1;
   private _offsetHeight = -1;

tests/convert-to-jest.js

@@ -74,12 +74,12 @@ utils.writeJSONFile(path.join(testSrc, 'tsconfig.json'), tsData);
 
 // Git remove old tests infra
 ['karma-cov.conf.js', 'karma.conf.js', 'run-test.py'].forEach(fname => {
-  utils.run(`git rm -f ./test-${name}/${fname}`);
+  utils.run(`git rm -f ./test-${name}/${fname} || true`);
 });
 
 // Copy common files from coreutils
 ['run.py', 'babel.config.js'].forEach(fname => {
-  fs.copySync(path.join(coreUtils, fname, path.join(testSrc, fname)));
+  fs.copySync(path.join(coreUtils, fname), path.join(testSrc, fname));
 });
 
 // Add new files to git

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

@@ -77,7 +77,7 @@ describe('terminal', () => {
 
   describe('.ISession', () => {
     describe('#terminated', () => {
-      it('should be emitted when the session is shut down', async () => {
+      it('should be emitted when the session is disposed', async () => {
         session = await TerminalSession.startNew();
         let called = false;
         session.terminated.connect((sender, args) => {
@@ -85,7 +85,7 @@ describe('terminal', () => {
           expect(args).to.be.undefined;
           called = true;
         });
-        await session.shutdown();
+        await session.dispose();
         expect(called).to.equal(true);
       });
     });

tests/test-terminal/run-test.py

@@ -1,10 +1,8 @@
 # Copyright (c) Jupyter Development Team.
 # Distributed under the terms of the Modified BSD License.
 
-import os
+import os.path as osp
 from jupyterlab.tests.test_app import run_karma
 
-HERE = os.path.realpath(os.path.dirname(__file__))
-
 if __name__ == '__main__':
-    run_karma(HERE)
+    run_karma(osp.dirname(osp.realpath(__file__)))

tests/test-terminal/src/terminal.spec.ts

@@ -57,7 +57,7 @@ describe('terminal/index', () => {
     });
 
     beforeEach(() => {
-      widget = new LogTerminal();
+      widget = new LogTerminal(session);
       Widget.attach(widget, document.body);
       return framePromise();
     });
@@ -73,13 +73,11 @@ describe('terminal/index', () => {
     });
 
     describe('#session', () => {
-      it('should be `null` by default', () => {
-        expect(widget.session).to.be.null;
+      it('should be the constructor value', () => {
+        expect(widget.session).to.equal(session);
       });
 
       it('should set the title when ready', async () => {
-        widget.session = session;
-        expect(widget.session).to.equal(session);
         await session.ready;
         expect(widget.title.label).to.contain(session.name);
       });
@@ -127,7 +125,6 @@ describe('terminal/index', () => {
 
     describe('#refresh()', () => {
       it('should refresh the widget', () => {
-        widget.session = session;
         return widget.refresh();
       });
     });
@@ -141,7 +138,6 @@ describe('terminal/index', () => {
 
     describe('#onAfterAttach()', () => {
       it('should post an update request', async () => {
-        widget.session = session;
         Widget.detach(widget);
         Widget.attach(widget, document.body);
         await framePromise();
@@ -151,7 +147,6 @@ describe('terminal/index', () => {
 
     describe('#onAfterShow()', () => {
       it('should post an update request', async () => {
-        widget.session = session;
         widget.hide();
         Widget.detach(widget);
         Widget.attach(widget, document.body);