From 45dbbb14328a173cad05ddcf21b5acf7f6460bb8 Mon Sep 17 00:00:00 2001
From: Shinigami <chrissi92@hotmail.de>
Date: Sun, 14 Feb 2021 00:05:23 +0100
Subject: [PATCH] Clean worker from pool after it was destroyed (#146)
MIME-Version: 1.0
Content-Type: text/plain; charset=utf8
Content-Transfer-Encoding: 8bit

Co-authored-by: Jérôme Benoit <jerome.benoit@sap.com>
---
 src/pools/abstract-pool.ts        | 7 +++----
 src/pools/cluster/dynamic.ts      | 1 -
 src/pools/cluster/fixed.ts        | 1 -
 src/pools/thread/dynamic.ts       | 1 -
 src/pools/thread/fixed.ts         | 1 -
 tests/pools/cluster/fixed.test.js | 2 +-
 6 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/src/pools/abstract-pool.ts b/src/pools/abstract-pool.ts
index 6e4465ed..f6ed280c 100644
--- a/src/pools/abstract-pool.ts
+++ b/src/pools/abstract-pool.ts
@@ -24,6 +24,7 @@ export interface IWorker {
   on(event: 'error', handler: ErrorHandler<this>): void
   on(event: 'online', handler: OnlineHandler<this>): void
   on(event: 'exit', handler: ExitHandler<this>): void
+  once(event: 'exit', handler: ExitHandler<this>): void
 }
 
 /**
@@ -161,9 +162,7 @@ export abstract class AbstractPool<
   }
 
   public async destroy (): Promise<void> {
-    for (const worker of this.workers) {
-      await this.destroyWorker(worker)
-    }
+    await Promise.all(this.workers.map(worker => this.destroyWorker(worker)))
   }
 
   /**
@@ -288,8 +287,8 @@ export abstract class AbstractPool<
 
     worker.on('error', this.opts.errorHandler ?? (() => {}))
     worker.on('online', this.opts.onlineHandler ?? (() => {}))
-    // TODO handle properly when a worker exit
     worker.on('exit', this.opts.exitHandler ?? (() => {}))
+    worker.once('exit', () => this.removeWorker(worker))
 
     this.workers.push(worker)
 
diff --git a/src/pools/cluster/dynamic.ts b/src/pools/cluster/dynamic.ts
index 6d665cef..432feb3a 100644
--- a/src/pools/cluster/dynamic.ts
+++ b/src/pools/cluster/dynamic.ts
@@ -68,7 +68,6 @@ export class DynamicClusterPool<
         if (message.kill) {
           this.sendToWorker(worker, { kill: 1 })
           void this.destroyWorker(worker)
-          this.removeWorker(worker)
         }
       })
       return worker
diff --git a/src/pools/cluster/fixed.ts b/src/pools/cluster/fixed.ts
index 0d8021eb..6620d5f3 100644
--- a/src/pools/cluster/fixed.ts
+++ b/src/pools/cluster/fixed.ts
@@ -60,7 +60,6 @@ export class FixedClusterPool<
 
   protected destroyWorker (worker: Worker): void {
     worker.kill()
-    // FIXME: The tests are currently failing, so these must be changed first
   }
 
   protected sendToWorker (worker: Worker, message: MessageValue<Data>): void {
diff --git a/src/pools/thread/dynamic.ts b/src/pools/thread/dynamic.ts
index c1033315..9ab6bf30 100644
--- a/src/pools/thread/dynamic.ts
+++ b/src/pools/thread/dynamic.ts
@@ -68,7 +68,6 @@ export class DynamicThreadPool<
         if (message.kill) {
           this.sendToWorker(worker, { kill: 1 })
           void this.destroyWorker(worker)
-          this.removeWorker(worker)
         }
       })
       return worker
diff --git a/src/pools/thread/fixed.ts b/src/pools/thread/fixed.ts
index 74c14ff2..bfd6f016 100644
--- a/src/pools/thread/fixed.ts
+++ b/src/pools/thread/fixed.ts
@@ -48,7 +48,6 @@ export class FixedThreadPool<
     worker: ThreadWorkerWithMessageChannel
   ): Promise<void> {
     await worker.terminate()
-    // FIXME: The tests are currently failing, so these must be changed first
   }
 
   protected sendToWorker (
diff --git a/tests/pools/cluster/fixed.test.js b/tests/pools/cluster/fixed.test.js
index aae9127f..d22f697a 100644
--- a/tests/pools/cluster/fixed.test.js
+++ b/tests/pools/cluster/fixed.test.js
@@ -120,7 +120,7 @@ describe('Fixed cluster pool test suite ', () => {
         closedWorkers++
       })
     })
-    pool.destroy()
+    await pool.destroy()
     await new Promise(resolve => setTimeout(resolve, 200))
     expect(closedWorkers).toBe(numberOfWorkers)
   })
-- 
2.34.1