Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Queue.pause(true) awaits only 1 active job, not all active jobs #1616

Closed
oavanruiten opened this issue Jan 14, 2020 · 3 comments · Fixed by #1586
Closed

Queue.pause(true) awaits only 1 active job, not all active jobs #1616

oavanruiten opened this issue Jan 14, 2020 · 3 comments · Fixed by #1586

Comments

@oavanruiten
Copy link

Description

Hi!

I was trying to figure out how to perform a graceful shutdown. The documentation states that when executing queue.pause(true), a promise is returned that resolves when the queue is paused. It will not process any new jobs and active jobs will continue until they complete. It relies on Queue#whenCurrentJobsFinished, which returns a promise that resolves when all jobs currently being processed by a given worker have finished.

Looking into Queue#whenCurrentJobsFinished, it appears that it does not wait for all active jobs to finish, but rather wait for a single job to complete.

/**
 * Returns a promise that resolves when active jobs are finished
 *
 * @returns {Promise}
 */
Queue.prototype.whenCurrentJobsFinished = function() {
  if (!this.bclientInitialized) {
    // bclient not yet initialized, so no jobs to wait for
    return Promise.resolve();
  }

  //
  // Force reconnection of blocking connection to abort blocking redis call immediately.
  //
  const forcedReconnection = redisClientDisconnect(this.bclient).then(() => {
    return this.bclient.connect();
  });

  return Promise.all([this.processing[0]]).then(() => {
    return forcedReconnection;
  });
};

Why not wait for all jobs in this.processing to finish before resolving?
I am currently using a concurrency value of 12, meaning that you would need to wait for at most 12 promises to resolve.

return Promise.all(this.processing).then(() => {
   return forcedReconnection;
});

This change would enable me to perform a perfect shutdown. Looking forward to hearing your thoughts!

@gabegorelick
Copy link
Contributor

This should be fixed by #1586

@oavanruiten
Copy link
Author

@gabegorelick Really appreciate it!

@oavanruiten
Copy link
Author

@manast Could you please create a new tag and npm release?

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 a pull request may close this issue.

2 participants