2
\$\begingroup\$

I wrote this mini thread pool in JavaScript that takes a maximum number of concurrently running tasks and runs them in the order it was given. I appreciate any feedback or review. Thank you.

class TaskPool {
  constructor(poolSize) {
    this.poolSize = poolSize;
    this.queue = [];
    this.runningTasks = 0;
    this.scheduled = null;
    this.id = 0;
  }

  backgroundRunner() {
    if (this.queue.length) {
      if (this.runningTasks < this.poolSize) {
        const index = 0;
        const [[task, id]] = this.queue.splice(index, 1);
        this.runningTasks++;
        task(id).finally(() => {
          this.runningTasks--;
          this.backgroundRunner();
        });
      } else {
        console.log('queue is full');
        if (!this.scheduled) {
          console.log('scheduled for later');
          this.scheduled = setTimeout(() => {
            this.backgroundRunner();
            this.scheduled = false;
          }, 1000);
        }
      }
    }
  }

  run(task) {
    this.queue.push([task, ++this.id]);
    this.backgroundRunner();
  }
}

Tests:

function task(id) {
  //create an ID
  return new Promise((resolve, _) => {
    console.log('running task #', id);
    setTimeout(() => {
      console.log('done running task #', id);
      resolve();
    }, 400);
  });
}

const pool = new TaskPool(3);
for (let i = 0; i < 10; i++) {
  pool.run(task);
}

jsfiddle

\$\endgroup\$
2
  • \$\begingroup\$ Maybe this steps from some confusion with promises? javascript is single threaded. promises are just a fancy alternative to using callback, and all callbacks execute on the same thread - one callback after another. Look up "Javascript event loop" to learn more about how this works. If you really want to do threading, you can use "web workers" to do the job. But a "promise pool" is just overhead that'll slow things down, not speed them up. \$\endgroup\$ Commented Dec 18, 2020 at 22:59
  • \$\begingroup\$ @ScottyJamison This was an interview question. I wanted to get feedback for my code \$\endgroup\$
    – Node.JS
    Commented Dec 19, 2020 at 0:44

2 Answers 2

3
\$\begingroup\$

Some suggestions:

  • It's not entirely clear until getting to the bottom of the class what the queue array holds. Maybe call it something more precise like unstartedTasks or unstartedTasksQueue.

  • You can return early to avoid nested indentation - cutting down on unnecessary indentation can make it clearer exactly which block a } at the end corresponds to. For example, backgroundRunner could be made to be:

    backgroundRunner() {
      if (!this.queue.length) {
        return;
      }
      // lots of code
    
  • The scheduled for later section looks superfluous since unstarted tasks will be run immediately upon the completion of a task, due to the

    task(id).finally(() => {
      this.runningTasks--;
      this.backgroundRunner();
    });
    
  • When you want to remove the first element from an array and use it, use shift, not splice. This:

    const [[task, id]] = this.queue.splice(index, 1);
    

    can be

    const [task, id] = this.queue.shift();
    
  • Unless the insertion index of the task needs to be saved, you could simplify the queue by incrementing and using the id not when the task is pushed, but when the task gets executed. This way the queue only needs to be an array of functions, not an array of [task, id]s.

See below for the version I'd prefer, if a class-based implementation is required. I use # to clearly distinguish what's exclusively private methods and properties from a public method:

class TaskPool {
  #unstartedTasksQueue = [];
  #runningTaskCount = 0;
  #id = 0;
  #runningTaskLimit;
  constructor(runningTaskLimit) {
    this.#runningTaskLimit = runningTaskLimit;
  }
  insertTask(fn) {
    if (this.#runningTaskCount === this.#runningTaskLimit) {
      this.#unstartedTasksQueue.push(task);
    } else {
      this.#startTask(fn);
    }
  }
  #startTask(fn) {
    this.#runningTaskCount++;
    fn(this.#id++).finally(() => {
      if (this.#unstartedTasksQueue.length) {
        this.#startTask(this.#unstartedTasksQueue.shift());
      } else {
        this.#runningTaskCount--;
      }
    });
  }
}

function task(id) {
  //create an ID
  return new Promise((resolve) => {
    console.log('running task #', id);
    setTimeout(() => {
      console.log('done running task #', id);
      resolve();
    }, 4000);
  });
}

const pool = new TaskPool(3);
for (let i = 0; i < 5; i++) {
  pool.insertTask(task);
}

But all the private methods and properties, despite making the code's intent a bit clearer, can look a bit ugly. Since the external user of the class is only interfacing with 2 things (the constructor, and the run method), I'd prefer to make a function instead of a class, which returns the run (or, as I've named it, insertTask) method, so as to rely on the closure instead of on #s:

const makeTaskPool = (runningTaskLimit) => {
  const unstartedTasksQueue = [];
  let runningTaskCount = 0;
  let id = 0;
  const startTask = (fn) => {
    runningTaskCount++;
    fn(id++).finally(() => {
      if (unstartedTasksQueue.length) {
        startTask(unstartedTasksQueue.shift());
      } else {
        runningTaskCount--;
      }
    });
  };
  return (fn) => {
    if (runningTaskCount === runningTaskLimit) {
      unstartedTasksQueue.push(task);
    } else {
      startTask(fn);
    }
  };
};

function task(id) {
  //create an ID
  return new Promise((resolve) => {
    console.log('running task #', id);
    setTimeout(() => {
      console.log('done running task #', id);
      resolve();
    }, 4000);
  });
}

const insertTask = makeTaskPool(3);
for (let i = 0; i < 5; i++) {
  insertTask(task);
}

\$\endgroup\$
2
\$\begingroup\$

Poor jargon leads to bugs

An important part of coding is correctly describing and/or interpreting from natural language what the code does or should do.

As natural language can be very ambiguous, descriptions use a lot of jargon that have very specific meaning within the context of programming, often completely different from the colloquial meaning.

If you are unsure about the correct use of programming jargon you should look it up.

Concurrent

From google

"Concurrent tasks are multiple tasks that progress at the same time but they are not executed simultaneously"

Thread

From google

"A thread is a unit of execution. Multithreading is a technique which allows a CPU to execute many tasks of one process at the same time. "

  • "Multithreading" and "Thread" are interchangeable in JavaScript (and most languages) as JavaScript does not provide a mechanism to distinguish the two.

  • In JavaScript a "Thread" has a specific property of being "Non blocking"

  • In JavaScript a "Thread" is AKA a "worker".

Bug #1

The question's title and question

"Mini JavaScript thread pool with maximum number of concurrently running tasks"

"I wrote this mini thread pool in JavaScript that takes a maximum number of concurrently running tasks..."

Threads run in parallel or concurrently via web workers. If I create a task I would expect your code to do as advertised.

function task(id) {
     return new Promise(completed => {
          while (Math.random() > 0.00001) {}
          completed();
     });
}

const pool = new TaskPool(3);
pool.run(task);  // Blocks for indeterminate time
pool.run(task);  // this line is not executed until previous task is complete

However as your code is not threaded and the tasks do not run concurrently, the first task blocking all execution until it resolves its promise.

Bug #2

Your code assumes that the task returns a promise. As you do not vet the argument task in run(task) { to make sure it is a function, and when you attempt to execute it, if it does not throw "task Is not a function" it will throw ".finally is not a function" if task does not return a promise.

Because the task count this.runningTasks++; is incremented before the error each time there is this error the pool size is permanently reduced by one.

Redundant code.

The polling of TaskPool.backgroundRunner

 else {
    console.log('queue is full');
    if (!this.scheduled) {
      console.log('scheduled for later');
      this.scheduled = setTimeout(() => {
        this.backgroundRunner();
        this.scheduled = false;
      }, 1000);
    }
  }

And property

  this.scheduled = false;

can be completely removed from the code without changing its behavior at all.

There is no need to poll backgroundRunner as the task.finally has a 1 to 1 match with completed tasks.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.