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

Oversight on task_progress event? #87

Closed
sbasher314 opened this issue Sep 7, 2022 · 5 comments
Closed

Oversight on task_progress event? #87

sbasher314 opened this issue Sep 7, 2022 · 5 comments

Comments

@sbasher314
Copy link

Hello, I am recently working on a project using better-queue. I presume that the progressTask function should be able to work as a partial (say, inserted 10/100 records) and should be able to display that a task / ticket is 10% complete. However, utilizing either progressTask or progressBatch causes the ticket to stop being tracked / updated - I believe the below code is why:

worker.on('task_progress', function (id, progress) {
    var taskId = taskIds[id];
    if (tickets[taskId]) {
      tickets[taskId].progress(progress);
      delete tickets[taskId];
    }
    self.emit('task_progress', taskId, progress);
  })

It looks like this is almost identical behavior to the worker.on('task_finish) listener, and may have erroneously been copied over:

worker.on('task_finish', function (id, result) {
    var taskId = taskIds[id];
    var stats = updateStatsForEndedTask(taskId);
    if (tickets[taskId]) {
      tickets[taskId].finish(result);
      delete tickets[taskId];
    }
    self.emit('task_finish', taskId, result, stats);
  })

For my purposes, simply commenting out / removing the line delete tickets[taskId] in the task_progress listener above resolves this issue, and I do not see it breaking expected behavior. I just want to make sure that this is not the intended behavior, or if this conflicts with anything in any way.

@leanderlee
Copy link
Member

leanderlee commented Sep 7, 2022 via email

@sbasher314
Copy link
Author

sbasher314 commented Sep 7, 2022

Sure -

Lets say I have some number of operations to complete for a single task. We will use a for loop for simplicity -

const iterationCount = 100
for (let i = 1;  i <= iterationCount; i++) {
  this.progressTask(0, i, iterationCount, 'iterations')
}

I would like to be able to know how many operations occurred in this task up to some point.

After the first progressTask usage, the ticket is deleted from the worker / queue and does not receive further updates, such as the final result, status, or eta. The other events still mostly appear to work as expected.

The context is that I am using a sort of ticket tracking system to store progress, results, status, etc, and refer to the original ticket object.

I could potentially rewrite the system to use a bunch of listeners instead, but I'm still not sure how deleting the current ticket on a progress event is beneficial (what if the ticket fails after progress has occured?)

@leanderlee
Copy link
Member

Hmm, yeah, I think you're right. This does seem like an oversight... Weird that no one caught this! Fix incoming

@leanderlee
Copy link
Member

Fix pushed to 3.8.11

@sbasher314
Copy link
Author

Awesome, thanks!

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

No branches or pull requests

2 participants