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

Ratelimiter does not keep process running until all requests are finished #16

Closed
bsingr opened this issue Sep 13, 2019 · 3 comments
Closed
Assignees
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@bsingr
Copy link

bsingr commented Sep 13, 2019

Problem example

Here it shows a Promise.all does not longer keep the process alive, it terminates... I guess this ratelimiter was implemented with daemonized scripts in mind?

const axios = require('axios')
const rl = require('axios-rate-limit');

// sets max 2 requests per 1 second, other will be delayed
const http = rl(axios.create(), { maxRequests: 2, perMilliseconds: 1000 });

Promise.all([
  http.get('http://httpbin.org/get').then(x => {console.log(1); return x}),
  http.get('http://httpbin.org/get').then(x => {console.log(2); return x}),
  http.get('http://httpbin.org/get').then(x => {console.log(3); return x}),
]).then(x => {
  console.log('after')
})

Problem workaround

If you count yourself how many requests you scheduled and if they are all finished or just add a global timeout to keep the process alive it works:

const axios = require('axios')
const rl = require('axios-rate-limit');

// sets max 2 requests per 1 second, other will be delayed
const http = rl(axios.create(), { maxRequests: 2, perMilliseconds: 1000 });

Promise.all([
  http.get('http://httpbin.org/get').then(x => {console.log(1); return x}),
  http.get('http://httpbin.org/get').then(x => {console.log(2); return x}),
  http.get('http://httpbin.org/get').then(x => {console.log(3); return x}),
]).then(x => {
  console.log('after')
})

setTimeout(function() {
  console.log('kept alive for 5s')
}, 5000)
@aishek aishek self-assigned this Sep 14, 2019
@aishek
Copy link
Owner

aishek commented Sep 15, 2019

No, the bug was introduced in #13. I just released version 1.1.3 with the fix. Could you confirm everything is working as expected now?

@aishek aishek added bug Something isn't working help wanted Extra attention is needed labels Sep 15, 2019
@bsingr
Copy link
Author

bsingr commented Sep 16, 2019

That works for me, thanks alot!

@aishek
Copy link
Owner

aishek commented Sep 16, 2019

If someone knows how to test ref and unref calls or it's sideeffects in Jest, please let me know. The PR is missing tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants