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

Limit multiple actions #61

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

huksley
Copy link
Contributor

@huksley huksley commented Nov 17, 2021

@peterkhayes Hi, here is the pretty naive implementation, would this work?

I think not, because ...Info() methods return always actionsRemaining == maxInInterval - 1, however, it is my expectation what there would be maxInterval available.

Could you take a look at this? I find all multithreaded programming extremely difficult to implement right.

Feeble attempt to fix #58

README.md Outdated Show resolved Hide resolved
src/index.ts Show resolved Hide resolved
src/index.ts Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
@peterkhayes
Copy link
Owner

Could you also update the readme, under the Instance Methods section, to add the new parameter?

@huksley
Copy link
Contributor Author

huksley commented Nov 18, 2021

It seems that actionsRemaining is broken for all operations - it always returns N-1, because of

I added also test at src/index.test.ts:107 which will not succeed even without my changes

    it('prevents more than maxInInterval actions within interval', async () => {
      const options = { interval: 10, maxInInterval: 2 };
      const limiter = await createLimiter(options);

      // Should allow first action through.
      setTime(0);
      // Should have 2 actions available at start
      expect(await limiter.wouldLimitWithInfo(id)).toEqual({
        actionsRemaining: 2,
        blocked: false,
        blockedDueToCount: false,
        blockedDueToMinDifference: false,
        millisecondsUntilAllowed: 0,
      });

At least that's how I expect library to behave, am I wrong?

if (addNewTimestamp) {
batch.zadd(key, String(now), uuid());
for (let i = 0; i < addNewTimestamps; i++) {
batch.zadd(key, String(now + i), uuid());
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice catch!

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.

How to consume multiple actions by calling limitWithInfo()?
2 participants