Skip to content

Fix scheduler termination promise semantics - #1070

Open
zloglevel wants to merge 1 commit into
naptha:masterfrom
zloglevel:master
Open

zloglevel wants to merge 1 commit into
naptha:masterfrom
zloglevel:master

Conversation

@zloglevel

Copy link
Copy Markdown

Summary

  • wait for all worker termination promises before resolving scheduler.terminate()
  • reject jobs that are still queued when the scheduler is terminated
  • add regression tests for both behaviors

Problem

scheduler.terminate() currently uses an async callback with
Array.prototype.forEach():

Object.keys(workers).forEach(async (wid) => {
  await workers[wid].terminate();
});

forEach() does not await the promises returned by its callback. As a result, await scheduler.terminate() can complete before the workers' termination promises have settled.

The method also clears jobQueue directly. Each queued entry closes over the resolve and reject functions of the Promise returned by addJob(). Removing those entries without rejecting them leaves the corresponding job promises permanently pending.

Solution

Store each queued job's run and reject functions. During termination:

  1. remove and reject all jobs that have not yet been assigned to a worker
  2. await all worker termination promises with Promise.all()

Queued jobs are rejected with a Scheduler terminated error.

Tests

  • verifies that scheduler.terminate() waits for worker termination
  • verifies that queued jobs reject instead of remaining pending
  • npm run lint
  • npm test

Signed-off-by: zloglevel <loglevel@outlook.com>
@zloglevel

Copy link
Copy Markdown
Author

@Balearica Could you please take a look when you have a chance? Thanks!

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant