From 6b155e60630028085d36d37be0cb6fcd1241d0b7 Mon Sep 17 00:00:00 2001 From: Steven Silvester Date: Sat, 11 May 2019 05:34:47 -0500 Subject: [PATCH 1/5] Handle disconnect messages from the terminal --- packages/services/src/terminal/default.ts | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/packages/services/src/terminal/default.ts b/packages/services/src/terminal/default.ts index 3c8b7e4292e9..f058f88e5a65 100644 --- a/packages/services/src/terminal/default.ts +++ b/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); } }; @@ -232,7 +238,7 @@ export class DefaultTerminalSession implements TerminalSession.ISession { } private _reconnectSocket(): void { - if (this._isDisposed || !this._ws) { + if (this._isDisposed || !this._ws || this._disconnected) { return; } @@ -277,6 +283,7 @@ export class DefaultTerminalSession implements TerminalSession.ISession { }; private _reconnectLimit = 7; private _reconnectAttempt = 0; + private _disconnected = false; } /** From 215aa21cfbb3efaf821f26fcbeacb761d24e2eed Mon Sep 17 00:00:00 2001 From: Steven Silvester Date: Sun, 12 May 2019 05:30:28 -0500 Subject: [PATCH 2/5] Clean up terminal shutdown behavior and tests --- packages/services/src/terminal/default.ts | 16 +---- packages/terminal-extension/src/index.ts | 31 ++++----- packages/terminal/src/constants.ts | 2 +- packages/terminal/src/widget.ts | 67 +++++++++---------- .../src/terminal/terminal.spec.ts | 4 +- tests/test-terminal/src/terminal.spec.ts | 11 +-- 6 files changed, 55 insertions(+), 76 deletions(-) diff --git a/packages/services/src/terminal/default.ts b/packages/services/src/terminal/default.ts index f058f88e5a65..5b8afab78b86 100644 --- a/packages/services/src/terminal/default.ts +++ b/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(); - } - } } diff --git a/packages/terminal-extension/src/index.ts b/packages/terminal-extension/src/index.ts index 67d82698c870..b5d253a941b9 100644 --- a/packages/terminal-extension/src/index.ts +++ b/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); diff --git a/packages/terminal/src/constants.ts b/packages/terminal/src/constants.ts index 75a8391dff67..d7a9218c64b7 100644 --- a/packages/terminal/src/constants.ts +++ b/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. diff --git a/packages/terminal/src/widget.ts b/packages/terminal/src/widget.ts index 2f49e8d40229..cc29c4188fd9 100644 --- a/packages/terminal/src/widget.ts +++ b/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 = {}) { + constructor( + session: TerminalSession.ISession, + options: Partial = {} + ) { 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 { - 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; diff --git a/tests/test-services/src/terminal/terminal.spec.ts b/tests/test-services/src/terminal/terminal.spec.ts index be7644901749..b4e3e000f886 100644 --- a/tests/test-services/src/terminal/terminal.spec.ts +++ b/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); }); }); diff --git a/tests/test-terminal/src/terminal.spec.ts b/tests/test-terminal/src/terminal.spec.ts index c74d6a1d98a9..ef30b626ee16 100644 --- a/tests/test-terminal/src/terminal.spec.ts +++ b/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); From df436755ba7a6838c696c6d7bb7354cf4bb74d36 Mon Sep 17 00:00:00 2001 From: Steven Silvester Date: Sun, 12 May 2019 06:30:17 -0500 Subject: [PATCH 3/5] cleanup --- tests/convert-to-jest.js | 4 ++-- tests/test-terminal/run-test.py | 6 ++---- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/tests/convert-to-jest.js b/tests/convert-to-jest.js index f345a3a9381d..bd5311a39e4b 100644 --- a/tests/convert-to-jest.js +++ b/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 diff --git a/tests/test-terminal/run-test.py b/tests/test-terminal/run-test.py index 173af144009e..56a4da8bdaa7 100644 --- a/tests/test-terminal/run-test.py +++ b/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__))) From 7ac3e23b35fee2e5a5cda93c0dfae8f9fce609ae Mon Sep 17 00:00:00 2001 From: Steven Silvester Date: Sun, 12 May 2019 06:36:21 -0500 Subject: [PATCH 4/5] cleanup term example --- examples/terminal/src/index.ts | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/examples/terminal/src/index.ts b/examples/terminal/src/index.ts index 0acfd1ea89d1..090112654970 100644 --- a/examples/terminal/src/index.ts +++ b/examples/terminal/src/index.ts @@ -12,22 +12,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 { let dock = new DockPanel(); - dock.addWidget(term1); - dock.addWidget(term2, { mode: 'tab-before' }); dock.id = 'main'; // Attach the widget to the dom. @@ -37,6 +23,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); From 469696d813599adea11cf8fa10f6ba5c354880c7 Mon Sep 17 00:00:00 2001 From: Steven Silvester Date: Mon, 13 May 2019 05:18:27 -0500 Subject: [PATCH 5/5] import dependent styles --- examples/terminal/src/index.ts | 1 + packages/application/style/index.css | 1 + 2 files changed, 2 insertions(+) diff --git a/examples/terminal/src/index.ts b/examples/terminal/src/index.ts index 090112654970..d078b548df02 100644 --- a/examples/terminal/src/index.ts +++ b/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'; diff --git a/packages/application/style/index.css b/packages/application/style/index.css index 8c46f8209d31..c86be5b18049 100644 --- a/packages/application/style/index.css +++ b/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;