Clean up terminal shutdown behavior and tests

packages/services/src/terminal/default.ts

@@ -232,6 +232,9 @@ export class DefaultTerminalSession implements TerminalSession.ISession {
 
       socket.onclose = (event: CloseEvent) => {
         console.warn(`Terminal websocket closed: ${event.code}`);
+        if (this._disconnected) {
+          this.dispose();
+        }
         this._reconnectSocket();
       };
     });
@@ -440,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);
     });
   }
 
@@ -495,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
     });
   }
 
@@ -274,27 +274,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);
     }
   });
 
@@ -304,7 +299,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/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/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);