-
Notifications
You must be signed in to change notification settings - Fork 6
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
UnhandledPromiseRejectionWarnings #8
Comments
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. |
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
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! |
This Promise is what 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 |
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 which actually doesn't have the catch method defined, so it ends up looking like this:
Pushing callbacks into the queue in this way does prevent the UnhandledPromiseRejection issues. Thanks again for taking a look at this, much appreciated! |
You're right, that should inherit from |
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. |
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. |
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!
The text was updated successfully, but these errors were encountered: