UnhandledPromiseRejectionWarnings

[hotfix519]

I'm using this lib for the first time, and I'm seeing these warnings somewhat frequently. Essentially any time I have a Promise task, and it's rejected, I see this warning. As well after attempting to cancel the queue.

Poking around with the code, I found that if you add a catch handler to the Promise created in the push method, these warnings no longer appear. It does also appear that the lib continues to function the way it's intended to, but I can't be 100% certain.

What do you suggest is the best way to avoid these errors/warnings? With unhandled promise rejections going to cause program termination in the future, it's a big issue!

Thanks!

[BalassaMarton]

If the promise is rejected within the task, I think it is expected behavior that the queue does not handle the rejection, the same way it won't catch and suppress any exceptions. Cancellation is another story. If you add a task to the queue, and await the returned Promise, what is the expected behavior after cancellation? It surely can't resolve the Promise, since the task was never executed. Rejecting seemed logical, since the Promise will not be resolved, ever.

[hotfix519]

Thanks for responding!

I agree with your assessment, that all makes sense. The problem to me is with the Promise that is created internally by the queue here

    push(task: Function, options?: TaskOptions): CancellablePromiseLike<any> {
        // snip Line - 166
        var result = (new Promise((resolve, reject) => {
            taskEntry.resolve = resolve;
            taskEntry.reject = reject;
        }) as any) as CancellablePromiseLike<any>;
       // snip
    }

This promise that is created here, has nothing handling its rejection. So when reject is called on this Promise from within the doneTask method, this triggers the UnhandledPromiseRejection warnings from the Node environment. In my quick messing around with it, adding a catch function to that promise prevented the warnings, and did not seem to affect the expected program flow. However, you're more familiar with this code and I wanted to see what you thought of the issue, perhaps there was something I missed.

If there's anything else I can do to help out on this, please let me know. Also, thanks for providing this code, it's helped out immensely!

[BalassaMarton]

This Promise is what push returns. I can see your dilemma, but if you don't handle rejections for the returned Promise, then any other rejection (typically from a timeout or an exception) will also stay unhandled and terminate the process. The semantics of a task cancellation is somewhat similar to a timeout - cancelling a task is like immediately setting it's timeout to 0. It's the caller's responsibility to decide if a cancellation is expected or not. If it is expected, then it should catch the Promise returned by push and examine the reason like this:

queue.push(() => { /* ... */ }).catch(reason => 
{
    if (reason === cancellationTokenReasons.cancel) { /* do nothing */ }
});

Note that this kind of error handling also works perfectly with async-await:

try {
    await queue.push(fn);
} 
catch (e) {
    if (e === cancellationTokenReasons.cancel) { /* do nothing */ }
}

One improvement I can think of is to get rid of the cancellationTokenReasons constants and use dedicated types instead, to allow instanceof checks, but this would be a breaking change.

[hotfix519]

Ahh, ok, I gotcha now, the user is supposed to handle that promise rejection.

In that case, I would suggest maybe adding this to the examples in the documentation. To make it a little clearer that we're responsible for doing that.

Keeping in mind that push returns CancellablePromiseLike<T> which actually doesn't have the catch method defined, so it ends up looking like this:

queue.push( callback ).then( undefined, catchHandler );

Pushing callbacks into the queue in this way does prevent the UnhandledPromiseRejection issues. Thanks again for taking a look at this, much appreciated!

[BalassaMarton]

You're right, that should inherit from Promise<T>. This whole cancellation feature is worth a discussion, maybe it is a bad pattern in its current form. I'd be happy to change it in a new major version, maybe more closely following .NET's CancellationToken semantics.

[hotfix519]

I'm not sure if I would say, bad, maybe just not quite intuitive enough. I think just with the layers of handling and queuing and etc. it's not obvious where somethings need to be handled and some don't. Which I think could hopefully be ironed out.

In regards to cancelling an entire queue, had you considered when a queue is cancelled, setting the cancel token, and then allowing the rest of the queue to execute? I'm thinking maybe if there were some cleanup or maintenance steps happening at the end of the task queue. You'd maybe want those tasks to run regardless if the entire queue has been cancelled, perhaps having different behaviour in each.

I was able to accomplish this by having this cleanup/maintenance happen when queue.wait() resolves. I just wanted to bring it up, because I feel it could be quite powerful if it worked that way, where a cancelled queue still ran all callbacks along with the cancelled token.

[BalassaMarton]

If you queue a task and care if it is rejected, you should get notified when it's cancelled, no matter if it was the only task that got cancelled or the entire queue. I think for now the recommended pattern is always handling rejections if they can crash the application.