fix(plugins/plugin-bash-like): new tab in browser-based clients may result in multiple new sessions

part of #7465

Author: starpit

plugins/plugin-bash-like/src/pty/client.ts

@@ -38,7 +38,7 @@ import {
 import Options from './options'
 import ChannelId from './channel-id'
 import { cleanupTerminalAfterTermination } from './util'
-import { getChannelForTab, invalidateSession } from './session'
+import { getChannelForTab, setChannelForTab, invalidateSession } from './session'
 import { Channel, InProcessChannel, WebViewChannelRendererSide } from './channel'
 
 const debug = Debug('plugins/bash-like/pty/client')
@@ -770,10 +770,12 @@ const getOrCreateChannel = async (
 
   const cachedws = getChannelForTab(tab)
 
-  if (!cachedws || cachedws.readyState === WebSocket.CLOSING || cachedws.readyState === WebSocket.CLOSED) {
-    // allocating new channel
-    const ws = await channelFactory(tab)
-    tab['ws'] = ws
+  if (!cachedws) {
+    // Then we need to allocate a new channel. BE CAREFUL: avoid races
+    // here by ensure that any `await` comes *after* grabbing a
+    // reference to the channelFactory promise. see
+    // https://github.com/kubernetes-sigs/kui/issues/7465
+    const ws = await setChannelForTab(tab, channelFactory(tab))
 
     // when the websocket is ready, handle any queued input; only then
     // do we focus the terminal (till then, the CLI module will handle
@@ -784,17 +786,15 @@ const getOrCreateChannel = async (
     const onClose = function(evt) {
       debug('channel has closed', evt.target, uuid)
       ws.removeEventListener('close', onClose)
-      if (!tab.state.closed) {
-        debug('attempting to reestablish connection, because the tab is still open')
-        invalidateSession(tab)
-      }
+      debug('attempting to reestablish connection, because the tab is still open')
+      invalidateSession(tab)
     }
     ws.on('close', onClose)
 
     return ws
   } else {
     // reusing existing websocket
-    doExec(cachedws)
+    doExec(await cachedws)
     if (terminal) {
       focus(terminal)
     }

plugins/plugin-bash-like/src/pty/server.ts

@@ -294,7 +294,7 @@ export const onConnection = (exitNow: ExitHandler, uid?: number, gid?: number) =
           const shell = msg.uuid && (await shells[msg.uuid])
           if (shell) {
             shell.kill(msg.signal || 'SIGHUP')
-            return exitNow(msg.exitCode || 0)
+            shells[msg.uuid] = undefined
           }
 
           const job = msg.uuid && jobs[msg.uuid]
@@ -307,6 +307,18 @@ export const onConnection = (exitNow: ExitHandler, uid?: number, gid?: number) =
         }
 
         case 'exit':
+          for (const uuid in shells) {
+            if (shells[uuid]) {
+              ;(await shells[uuid]).kill('SIGHUP')
+              shells[uuid] = undefined
+            }
+          }
+          for (const uuid in jobs) {
+            if (jobs[uuid]) {
+              jobs[uuid].abort()
+              jobs[uuid] = undefined
+            }
+          }
           return exitNow(msg.exitCode || 0)
 
         case 'request': {
@@ -565,9 +577,10 @@ export default (commandTree: Registrar) => {
       // eslint-disable-next-line no-async-promise-executor
       new Promise(async (resolve, reject) => {
         try {
-          await new StdioChannelKuiSide().init(() => {
+          await new StdioChannelKuiSide().init((exitCode: number) => {
             debug('done with stdiochannel')
-            resolve(true)
+            process.exit(exitCode)
+            // resolve(true)
           })
         } catch (err) {
           reject(err)

plugins/plugin-bash-like/src/pty/session.ts

@@ -30,8 +30,34 @@ const debug = Debug('plugins/bash-like/pty/session')
  * Return the cached websocket for the given tab
  *
  */
-export function getChannelForTab(tab: Tab): Channel {
-  return tab['ws'] as Channel
+let _singleChannel: Promise<Channel> // share session across tabs see https://github.com/IBM/kui/issues/6453
+let _exiting = false
+// eslint-disable-next-line @typescript-eslint/no-unused-vars
+export function getChannelForTab(tab: Tab): Promise<Channel> {
+  if (_exiting) {
+    // prevent any stagglers re-establishing a channel
+    throw new Error('Exiting')
+  } else {
+    return _singleChannel
+  }
+}
+
+/**
+ * Set it
+ *
+ */
+export function setChannelForTab(tab: Tab, channel: Promise<Channel>) {
+  _singleChannel = channel
+
+  // listen for the end of the world
+  channel.then(chan => {
+    window.addEventListener('beforeunload', () => {
+      _exiting = true // prevent any stagglers re-establishing a channel
+      chan.close()
+    })
+  })
+
+  return _singleChannel
 }
 
 /**
@@ -80,6 +106,7 @@ export function pollUntilOnline(tab: Tab) {
           if (err.code !== 503) {
             // don't bother complaining too much about connection refused
             console.error('error establishing session', err.code, err.statusCode, err)
+            _singleChannel = undefined
           }
           setTimeout(() => once(iter + 1), iter < 10 ? 2000 : iter < 100 ? 4000 : 10000)
           return strings('Could not establish a new session')
@@ -95,7 +122,6 @@ export function pollUntilOnline(tab: Tab) {
  * given tab
  *
  */
-let _singleChannel: Promise<Channel> // share session across tabs see https://github.com/IBM/kui/issues/6453
 async function newSessionForTab(tab: Tab) {
   const thisSession =
     _singleChannel ||
@@ -104,15 +130,12 @@ async function newSessionForTab(tab: Tab) {
         await pollUntilOnline(tab)
         tab.classList.add('kui--session-init-done')
 
-        resolve(getChannelForTab(tab))
+        resolve(await getChannelForTab(tab))
       } catch (err) {
         reject(err)
       }
     })
 
-  if (!_singleChannel) {
-    _singleChannel = thisSession
-  }
   tab['_kui_session'] = thisSession
 
   await thisSession
@@ -138,24 +161,6 @@ export async function init(registrar: PreloadRegistrar) {
 
   if (inBrowser() && (proxyServer as { enabled?: boolean }).enabled !== false) {
     debug('initializing pty sessions')
-
-    const { eventBus } = await import('@kui-shell/core')
-
     registrar.registerSessionInitializer(newSessionForTab)
-
-    // listen for closed tabs
-    eventBus.on('/tab/close', async (tab: Tab) => {
-      try {
-        debug('closing session for tab')
-        const channel = getChannelForTab(tab)
-        if (channel) {
-          // the user may have asked to close the tab before we have
-          // finished initializing the channel
-          channel.close()
-        }
-      } catch (err) {
-        console.error('error terminating session for closed tab', err)
-      }
-    })
   }
 }

plugins/plugin-bash-like/src/pty/stdio-channel.ts

@@ -63,7 +63,9 @@ export class StdioChannelWebsocketSide extends EventEmitter implements Channel {
       ws.on('message', (data: string) => {
         debugW('forwarding message downstream')
         try {
-          child.stdin.write(`${data}${MARKER}`)
+          if (!child.stdin.destroyed) {
+            child.stdin.write(`${data}${MARKER}`)
+          }
         } catch (err) {
           console.error('error in child write', err)
         }
@@ -74,7 +76,15 @@ export class StdioChannelWebsocketSide extends EventEmitter implements Channel {
 
       ws.on('close', () => {
         debugW('killing child process, because client connection is dead')
-        child.kill()
+        try {
+          if (!child.stdin.destroyed) {
+            const data = JSON.stringify({ type: 'exit' })
+            child.stdin.write(`${data}${MARKER}`)
+          }
+        } catch (err) {
+          console.error('error in child write', err)
+        }
+        // no: child.kill()  <-- instead, send an exit event, to allow subprocess to clean up
       })
     })
 

plugins/plugin-bash-like/src/pty/websocket-channel.ts

@@ -43,7 +43,7 @@ class WebSocketChannel implements Channel {
   close() {
     try {
       debug('closing websocket channel')
-      this.ws.close()
+      this.ws.send(JSON.stringify({ type: 'exit' }))
     } catch (err) {
       console.error('Error in WebSocketChannel.close', err)
     }

plugins/plugin-client-common/src/components/Views/Terminal/ScrollableTerminal.tsx

@@ -915,6 +915,11 @@ export default class ScrollableTerminal extends React.PureComponent<Props, State
       eventChannelUnsafe.off(`/terminal/clear/${state.uuid}`, clear)
     })
 
+    // terminate watchable jobs when the window is closed
+    const termAll = this.terminateAllWatchables.bind(this)
+    window.addEventListener('beforeunload', termAll)
+    this.cleaners.push(() => window.removeEventListener('beforeunload', termAll))
+
     return state
   }
 
@@ -995,6 +1000,7 @@ export default class ScrollableTerminal extends React.PureComponent<Props, State
 
   public componentWillUnmount() {
     this.uninitEvents()
+    this.terminateAllWatchables()
   }
 
   /**
@@ -1018,7 +1024,7 @@ export default class ScrollableTerminal extends React.PureComponent<Props, State
     return availableSplitIdx
   }
 
-  // If the block has watchable response, abort the job
+  /** If the block has watchable response, abort the job */
   private removeWatchableBlock(block: BlockModel) {
     if (isOk(block)) {
       if (isWatchable(block.response)) {
@@ -1029,6 +1035,13 @@ export default class ScrollableTerminal extends React.PureComponent<Props, State
     }
   }
 
+  /** As per `removeWatchableBlock`, but for all blocks */
+  private terminateAllWatchables() {
+    this.state.splits.forEach(scrollback => {
+      scrollback.blocks.forEach(_ => this.removeWatchableBlock(_))
+    })
+  }
+
   /**
    * Remove the given split (identified by `sbuuid`) from the state.
    *