fix: destroy issue and use maximum workers #44

Aslemammad · 2023-01-13T17:07:28Z

Resolves #43
Resolves #42

@overlookmotel I did the maximumWorkers trick, but I do not have any reproduction for the halting issue, I'd like to see that so I'd write a proper test case for that.

Aslemammad · 2023-01-13T17:45:48Z

test/simple.test.ts

@@ -263,3 +264,22 @@ test('workerId should never be duplicated', async () => {
  await pool.destroy()
  await sleep(5000)
 }, 30000)
+
+/* test('isolateWorkers: true with minThreads of 0 should not halt(#42)', async () => {
+  const minThreads = 0,


@overlookmotel see this bit, it's 0 but it does not halt.

Thanks for working on this. I'll try to put together a failing test. I definitely saw this halting behavior in "real world". But give me a few days - work is crazy at the moment.

Ah I think I know what it is. This bug occurs when you have tasks waiting in the queue when a task completes. It never drains the queue.

If you add maxThreads + 1 tasks to the queue synchronously, then you'll see the last task never gets run.

const promises = []; for (let i = 0; i < maxThreads + 1; i++) { promises.push( pool.run({}) ); } const promiseForAll = Promise.all(promises); await promiseForAll; // `promiseForAll` never resolves

I just tried to create a PR with failing test, but I've already forked piscina so Github won't let me fork tinypool too - you can't fork a fork as well as the origin, it seems.

overlookmotel · 2023-01-13T23:42:12Z

test/fixtures/sleep.js

@@ -1,6 +1,6 @@
 'use strict'


I think you can remove 'use strict' if this file is ESM.

overlookmotel · 2023-01-14T00:35:12Z

src/index.ts

@@ -843,7 +843,7 @@ class ThreadPool {
        // When `isolateWorkers` is enabled, remove the worker after task is finished
        if (this.options.isolateWorkers && taskInfo.workerInfo) {
          this._removeWorker(taskInfo.workerInfo)
-          this._ensureMinimumWorkers()
+          this._ensureMaximumWorkers()


This will solve the problem, but will create more workers than required.

const pool = new Tinypool({ filename: 'worker.js', minThreads: 0, maxThreads: 8, isolateWorkers: true, }); await pool.run({});

In above example:

Pool is created with no workers initially.

pool.run() creates 1 worker to run the task.

When task completes, _ensureMaximumWorkers() creates 8 new workers.

None of these workers are ever given a task.

They'll shut themselves down again shortly after starting (depending on idleTimeout option).

A more ideal solution would be that when a task completes:

Check if either:

there are any tasks in the queue which don't already have a worker being started for them

or number of workers is less than minThreads.

If neither condition is true, do nothing.

If is either condition is true, create only 1 new worker.

When that worker starts it will run the next queued task (if there is one).

The first step is the tricky part. I don't think it's as simple as this.taskQueue.length > 0, because of circumstance where there are tasks in the queue, but they all already have a new worker being started for them.

I tried, but I can't figure out the logic of how a task is marked as "awaiting worker to start".

Aslemammad · 2023-01-15T06:29:06Z

src/index.ts

+  _ensureEnoughWorkersForTaskQueue(): void {
+    while (
+      this.workers.size < this.taskQueue.size &&
+      this.workers.size < this.options.maxThreads
+    ) {
+      this._addNewWorker()
+    }
+  }


@overlookmotel Do you think this solution is an optimal one?

I believe it's not wrong to have workers for those that already have since we do not have info! And I think on the second condition, it creates a limit for the workers and can be helpful. I'd like to see your opinion.

@overlookmotel Let me know if you're there.

I'm really sorry but I can't help more. As I mentioned on #42, I am very tight for time. The complexity (which we're discussing here) was exactly why I didn't feel I could make a PR myself. I find piscina's code pretty hard to disentangle.

But my guess is that your solution above is not quite right, and will create new workers when none is actually needed in some case.

My suggestion would be to look at the existing code for how it's handled when a thread is terminated due to use of an AbortController. That results in same situation (worker removed from pool, do we need to add a new one?) so I imagine the code for that contains a good solution.

sure! ok, thank you so much! I'll investigate it.

In AbortController the ensureMin approach is used which is not proper for our case!
I think let's stick with the _ensureEnoughWorkersForTaskQueue approach for now and let's see how it goes! Thanks for yout help.

purepear · 2023-01-28T16:15:04Z

Hi there 👋 , I followed your tweet and tested the latest Vitest 0.28.3 which uses tinypool 0.3.1.

I raised this PR that adds a new GHA workflow:

the tests run using 1 worker(visually observed the tests go one by one) and it took 1m 16s
I set minThreads and maxThreads to 2 and ran the tests again and this time it took 50s and I can observe the tests running 2 by 2.

  maxThreads: process.env.CI ? 2 : undefined,
  minThreads: process.env.CI ? 2 : undefined,

Let me know if I can further help with the debugging.

Aslemammad added 2 commits January 13, 2023 20:33

use maximum workers for isolate

25cc93b

WIP: cannot reproduce the halt issue

26f58ae

Aslemammad changed the base branch from current to main January 13, 2023 17:08

Aslemammad commented Jan 13, 2023

View reviewed changes

overlookmotel reviewed Jan 13, 2023

View reviewed changes

test/fixtures/sleep.js Outdated

@@ -1,6 +1,6 @@

'use strict'

Copy link

overlookmotel Jan 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can remove 'use strict' if this file is ESM.

overlookmotel reviewed Jan 14, 2023

View reviewed changes

Aslemammad added 4 commits January 15, 2023 07:38

fix destroy issue

c3bccaa

trace warning

ae42cb2

update

0a60bdd

WIP:_ensureEnouhWorkersForTaskQueue

34a204d

Aslemammad commented Jan 15, 2023

View reviewed changes

update test

2e304d0

Aslemammad linked an issue Jan 26, 2023 that may be closed by this pull request

isolateWorkers option can result in not utilizing all CPUs #42

Closed

Aslemammad merged commit 03fbfdf into main Jan 26, 2023

Aslemammad deleted the fix-destroy-and-maximum-workers branch January 26, 2023 21:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: destroy issue and use maximum workers #44

fix: destroy issue and use maximum workers #44

Aslemammad commented Jan 13, 2023 •

edited

Loading

Aslemammad Jan 13, 2023

overlookmotel Jan 13, 2023

overlookmotel Jan 14, 2023 •

edited

Loading

overlookmotel Jan 13, 2023

overlookmotel Jan 14, 2023 •

edited

Loading

Aslemammad Jan 15, 2023

Aslemammad Jan 15, 2023

Aslemammad Jan 19, 2023

overlookmotel Jan 19, 2023

Aslemammad Jan 20, 2023

Aslemammad Jan 26, 2023

purepear commented Jan 28, 2023

fix: destroy issue and use maximum workers #44

fix: destroy issue and use maximum workers #44

Conversation

Aslemammad commented Jan 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

overlookmotel Jan 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

overlookmotel Jan 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

purepear commented Jan 28, 2023

Aslemammad commented Jan 13, 2023 •

edited

Loading

overlookmotel Jan 14, 2023 •

edited

Loading

overlookmotel Jan 14, 2023 •

edited

Loading