mcollina / fastq

Fast, in memory work queue
ISC License
932 stars 47 forks source link

What if worker rejects? #53

Closed aguegu closed 3 years ago

aguegu commented 3 years ago

I created a promised queue, fastq.promise(), with an async worker that may go sideways. So as in regular promise functions, throwing a error would be enough.

const worker = async () => {
    throw new Error('worker' failed);
};

But the queue failed to catch it and I would get UnhandledPromiseRejectionWarning. Another fact is that the errorHandler does not catch it, even it is triggered on every single run, as described on #41 .

aguegu commented 3 years ago

After a source code review, I think the problem is about making push or unshifta promise, or function with callback.

I used to believe push or unshift would always work. And I do not need to (a)wait for the work to be done to get results. Because the waiting could take a while. That is what a task queue all about, right ? The errorHandler, fastq.errer, to catch/log the worker outcome is enough.

If the push or unshift do fail/reject, the reason would be something like the queue is too full to take anymore, not how the worker does its own job.

aguegu commented 3 years ago

It turns out that errorHandler catches the error.

queue.error((err, task) => {    
    console.error(err.message);
    console.log(task);
});

console.log(err); would print out nothing, only {}, because it is an error object. But this errorHandler would not stop the calling of the callback function of push or unshift function.

So I still get the UnhandledPromiseRejectionWarning.

Well, I think I would catch the error from push or unshift to suppress this warning. Like this,

queue.push(job).catch(() => {});

An option, or a flag, could be helpful in this case.

aguegu commented 3 years ago

@mcollina , Thanks for quick response. now v 1.12.0 works as I expected. Without (a)wating for the push action, the errorHandler would catch the failure from the worker. And I can remove the .catch(() => {}) part now.

Well, I am a new user to fastq, hope it will not break old users' instinct usage.

Good Job.

mcollina commented 3 years ago

I'm glad you liked the fix. I thought about this for a while and it made sense to just add the .catch() handler here. Most fastq users are not interested in the result.