Fix experiment stop button (#1731)

* stop experiment process with tree-kill because of dvc child processes and the way they handle keyboard interrupt

* move disposed event inside of kill callback

Author: mattseddon

extension/package.json

@@ -1384,7 +1384,8 @@
     "lodash.isempty": "^4.4.0",
     "lodash.isequal": "^4.5.0",
     "lodash.merge": "^4.6.2",
-    "lodash.omit": "^4.5.0"
+    "lodash.omit": "^4.5.0",
+    "tree-kill": "^1.2.2"
   },
   "devDependencies": {
     "@types/chai": "^4.2.22",

extension/src/cli/runner.ts

@@ -133,18 +133,26 @@ export class CliRunner extends Disposable implements ICli {
     )
   }
 
-  public async stop() {
+  public stop() {
+    const stopped = new Promise<boolean>(resolve => {
+      if (!this.currentProcess) {
+        return resolve(false)
+      }
+      const listener = this.dispose.track(
+        this.currentProcess.onDidDispose(disposed => {
+          this.dispose.untrack(listener)
+          listener?.dispose()
+          this.pseudoTerminal.close()
+          resolve(disposed)
+        })
+      )
+    })
+
     try {
       this.currentProcess?.dispose()
-      await this.currentProcess
-      return false
-    } catch {
-      const stopped = !this.currentProcess || this.currentProcess.killed
-      if (stopped) {
-        this.pseudoTerminal.close()
-      }
-      return stopped
-    }
+    } catch {}
+
+    return stopped
   }
 
   public isExperimentRunning() {

extension/src/processExecution.ts

@@ -1,5 +1,6 @@
 import { ChildProcess } from 'child_process'
 import { Readable } from 'stream'
+import { Event, EventEmitter } from 'vscode'
 import { Disposable } from '@hediet/std/disposable'
 import execa from 'execa'
 import { Logger } from './common/logger'
@@ -16,7 +17,9 @@ interface ProcessResult {
   signal?: string
 }
 
-export type Process = RunningProcess & Promise<ProcessResult> & Disposable
+export type Process = RunningProcess &
+  Promise<ProcessResult> &
+  Disposable & { onDidDispose: Event<boolean> }
 
 export interface ProcessOptions {
   executable: string
@@ -39,10 +42,14 @@ export const createProcess = ({
     windowsHide: true
   })
 
+  const disposed = new EventEmitter<boolean>()
+
   return Object.assign(process, {
     dispose: () => {
-      process.kill('SIGINT')
-    }
+      const kill = require('tree-kill')
+      kill(process.pid, 'SIGINT', () => disposed.fire(true))
+    },
+    onDidDispose: disposed.event
   })
 }
 

extension/src/test/suite/cli/runner.test.ts

@@ -56,8 +56,11 @@ suite('CLI Runner Test Suite', () => {
         true
       )
 
-      const stopped = await cliRunner.stop()
-      expect(stopped).to.be.true
+      // eslint-disable-next-line @typescript-eslint/no-explicit-any
+      const closeSpy = spy((cliRunner as any).pseudoTerminal, 'close')
+
+      await cliRunner.stop()
+      expect(closeSpy).to.be.calledOnce
 
       await completed
 

yarn.lock

@@ -13837,6 +13837,11 @@ tr46@~0.0.3:
   resolved "https://registry.yarnpkg.com/traverse/-/traverse-0.3.9.tgz#717b8f220cc0bb7b44e40514c22b2e8bbc70d8b9"
   integrity sha1-cXuPIgzAu3tE5AUUwisui7xw2Lk=
 
+tree-kill@^1.2.2:
+  version "1.2.2"
+  resolved "https://registry.yarnpkg.com/tree-kill/-/tree-kill-1.2.2.tgz#4ca09a9092c88b73a7cdc5e8a01b507b0790a0cc"
+  integrity sha512-L0Orpi8qGpRG//Nd+H90vFB+3iHnue1zSSGmNOOCh1GLJ7rUKVwV2HvijphGQS2UmhUZewS9VgvxYIdgr+fG1A==
+
 trim-trailing-lines@^1.0.0:
   version "1.1.4"
   resolved "https://registry.yarnpkg.com/trim-trailing-lines/-/trim-trailing-lines-1.1.4.tgz#bd4abbec7cc880462f10b2c8b5ce1d8d1ec7c2c0"