Skip to content

Limit multiple actions - #61

Open
huksley wants to merge 4 commits into
peterkhayes:masterfrom
huksley:multiple-actions
Open

Limit multiple actions#61
huksley wants to merge 4 commits into
peterkhayes:masterfrom
huksley:multiple-actions

Conversation

@huksley

@huksley huksley commented Nov 17, 2021

Copy link
Copy Markdown
Contributor

@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

Comment thread README.md Outdated
Comment thread src/index.ts
Comment thread src/index.ts
Comment thread src/index.ts Outdated
@peterkhayes

Copy link
Copy Markdown
Owner

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

@huksley

huksley commented Nov 18, 2021

Copy link
Copy Markdown
Contributor Author

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?

Comment thread src/index.ts
if (addNewTimestamp) {
batch.zadd(key, String(now), uuid());
for (let i = 0; i < addNewTimestamps; i++) {
batch.zadd(key, String(now + i), uuid());

Copy link
Copy Markdown
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