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

fix(clearQueue): reject pending promises #27

Closed

Conversation

jedwards1211
Copy link

Here's what I meant in #25

BREAKING CHANGE: when clearQueue() is called, promises for queued function calls will be rejected.

fix #25

test.js Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

@copperwall Any thoughts on this? I need to get this merged or closed before releasing a new version.

@copperwall
Copy link
Contributor

I think it looks pretty good. I tested it out by running a bunch of axios requests with a concurrency and running the clearQueue function in a setTimeout that ran after two seconds as a sort of timeout on the queue. The changes are helpful if you're awaiting a Promise.all in an async function, since now the promise returned by Promise.all will settle and you can handle it in the application code with a try/catch in an async function or a .catch handler. With the same example on master, the it looks like the promise returned from Promise.all doesn't ever settle.

I resolved the merge conflicts in a branch with the same name on my fork if you'd like to merge that in. https://github.com/copperwall/p-limit/tree/clear-queue-reject

index.js Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

// @callmehiphop

BREAKING CHANGE: when clearQueue() is called, promises for queued function calls will be rejected.

fix sindresorhus#25
@jedwards1211
Copy link
Author

okay, I made the requested changes and rebased

@callmehiphop
Copy link
Contributor

Would this better served as a separate method, like rejectPending or something?

@jedwards1211
Copy link
Author

jedwards1211 commented May 26, 2020

I don't really like the idea of never settling a promise, it will cause confusion. I think we should reject the pending promises by default and if the user wants to leave them unsettled (what for though?) they should have to opt into that.

@callmehiphop
Copy link
Contributor

This is really only an issue if people are actually waiting for the promise returned by limit() to be resolved. I think it is perfectly reasonable to add an async function to the limiter that includes error handling without needing to await anything returned by the limiter. However I can definitely see where some one might get thrown off by this, so I guess I'm kind of torn here. 🤷‍♂️

@jedwards1211
Copy link
Author

jedwards1211 commented May 26, 2020

I was looking at using this library to serialize queries on an MSSQL connection, which required the query function to return the promise from limit(). We ended up just deciding to use our own code to serialize those queries in sequelize. Do you think there would be any benefit to not settling the promises? (slight performance improvement I guess?)

Base automatically changed from master to main January 23, 2021 18:04
@sindresorhus
Copy link
Owner

#25 (comment)

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 this pull request may close these issues.

Should clearQueue() reject the pending promises?
5 participants