The sorted set holds the old request data

[amanda94]

I have read the code, there is something I do not understand. I know that this module using sorted set in redis to save the request time in ms . But I do not see some code for delete the old ones . I see the whole logic is this part of code:

const res = await db
      .multi()
      .zrange([key, 0, start, 'WITHSCORES'])
      .zcard([key])
      .zadd([key, now, now])
      .zrange([key, 0, 0])
      .pexpire([key, duration])
      .exec()

zcard is always reading the whole set when the set data is not expired. And every time the request comes, the set can not expire. So if I set a duration of 10 min and max count for 10 and request is send to server with same id for two mins a time and the set data is always growing when it reach the max count ,which is clearly not what we want. Hope you can understand my poor English

[Kikobeats]

I think you are right, it's related with https://github.com/tj/node-ratelimiter/issues/43

[amanda94]

I see the related one, but I do not think they are the same problems. My point is this logic is caculating the request times extend the duration since every times you reading the whole sorted set and not deleting the ones older than the duration time passed. for example: I have a limit 5 for duration 5, and I request the server for 1 in 2, which clearly should pass the limit . But the process will be

time	sorted set	remain times
t0	[0]	4
t2	[0, 2]	3
t4	[0,2,4]	2
t6	[0,2,4,6]	1
t8	[0,2,4,6,8]	0

so at t8 , the request will be rejected. I think before you caculate the set item count, you should delete the old items eariler then the current time mins duration time

[Kikobeats]

Hmm can you write the same example but from a test? It can reflect better what should be the final state and then we can change the implementation for passing it 🙂

[amanda94]

How about this ?