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

Callbacks processed out of order? #50

Open
kpruden opened this issue Feb 1, 2018 · 4 comments
Open

Callbacks processed out of order? #50

kpruden opened this issue Feb 1, 2018 · 4 comments

Comments

@kpruden
Copy link

kpruden commented Feb 1, 2018

It looks like maybe the callbacks provided to removeTokens can be called out of order. I'm not sure if ordering is an intended feature, but if not it'd be good call that out clearly in the docs.

var RateLimiter = require('limiter').RateLimiter;
var l = new RateLimiter(1, 10000);
var i = 0;
var f = () => {
  var j = i++;
  var d = new Date();
  l.removeTokens(1, () => console.log(`${j}: enqueued: ${d}; dequeued: ${new Date()}`));
}
for (var k = 0; k < 5; k++) setTimeout(() => f(), 1000*k);

Running this code several times shows arbitrary ordering:

> i = 0
> for (var k = 0; k < 5; k++) setTimeout(() => f(), 1000*k);
0: enqueued: Thu Feb 01 2018 12:41:18 GMT-0800 (PST); dequeued: Thu Feb 01 2018 12:41:18 GMT-0800 (PST)
2: enqueued: Thu Feb 01 2018 12:41:20 GMT-0800 (PST); dequeued: Thu Feb 01 2018 12:41:28 GMT-0800 (PST)
1: enqueued: Thu Feb 01 2018 12:41:19 GMT-0800 (PST); dequeued: Thu Feb 01 2018 12:41:38 GMT-0800 (PST)
3: enqueued: Thu Feb 01 2018 12:41:21 GMT-0800 (PST); dequeued: Thu Feb 01 2018 12:41:48 GMT-0800 (PST)
4: enqueued: Thu Feb 01 2018 12:41:22 GMT-0800 (PST); dequeued: Thu Feb 01 2018 12:41:58 GMT-0800 (PST)

> i = 0
> for (var k = 0; k < 5; k++) setTimeout(() => f(), 1000*k);
0: enqueued: Thu Feb 01 2018 12:45:00 GMT-0800 (PST); dequeued: Thu Feb 01 2018 12:45:00 GMT-0800 (PST)
1: enqueued: Thu Feb 01 2018 12:45:01 GMT-0800 (PST); dequeued: Thu Feb 01 2018 12:45:10 GMT-0800 (PST)
3: enqueued: Thu Feb 01 2018 12:45:03 GMT-0800 (PST); dequeued: Thu Feb 01 2018 12:45:20 GMT-0800 (PST)
4: enqueued: Thu Feb 01 2018 12:45:04 GMT-0800 (PST); dequeued: Thu Feb 01 2018 12:45:30 GMT-0800 (PST)
2: enqueued: Thu Feb 01 2018 12:45:02 GMT-0800 (PST); dequeued: Thu Feb 01 2018 12:45:40 GMT-0800 (PST)

I can work around this limitation so this isn't an urgent issue for me. However if I'm misunderstanding something any pointers would be helpful. I have a slight suspicion that maybe setTimeout doesn't quite work the way I think it does :)

@thinkhy
Copy link

thinkhy commented Mar 11, 2018

Wondering if there is any solution for this problem? I havn't thought out workaround in my scenario ...

@jhurliman
Copy link
Owner

Thank you for calling this out. This is due to using setTimeout to queue pending callbacks on the event loop rather than maintaining an explicitly ordered queue. Several timeouts will expire as soon as enough tokens are available and there is a race to see which one claims the token(s) first, while the rest set new timeouts.

If someone wants to take a stab at refactoring this I would be happy to review / provide feedback / merge.

@animir
Copy link

animir commented Jul 14, 2019

@kpruden @thinkhy @jhurliman
If you still looking for solution, rate-limiter-flexible package supports FIFO queue rate limiter. There is also migration guide, if you wish to migrate from limiter.

@LandonSchropp
Copy link

I'm not in love with this solution, but I was able to order by calls by ignoring the execution order of limiter and manually increment an index. I thought I'd post it here in case somebody else finds this approach useful.

/**
 * This performs concurrent operations using the provided limiter. This function ensures the array
 * is processed in a FIFO order, which is not a feature of the limiter library.
 */
async function mapWithRateLimiter(array, limiter, callback) {

  // We can't rely on the call order with limiter, so we have to manually increment the index.
  let result = [];
  let index = 0;

  // Run the limiter the necessary number of times.
  await Promise.all(_.times(array.length, async () => {
    await limiter.removeTokens(1);

    let currentIndex = index;
    index++;
    result[currentIndex] = await callback(array[currentIndex], currentIndex);
  }));

  return result;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants