Core bugs in codebase

[niftylettuce]

This codebase has core bugs.

1) With parseInt, you should specify radix of 10. parseInt(x, 10) (we are using whole integers, this makes sense) 2) You are checking that the database result has a value already when no value is yet set, e.g. [1] (see below).

const res = await this.db.multi(operations).exec()
const count = parseInt(res[1][1])
const oldest = parseInt(res[decrease ? 3 : 2][1])
const oldestInRange = parseInt(res[decrease ? 4 : 3][1])

[niftylettuce]

The bug is that the error will be thrown Cannot read property '1' of undefined

[Kikobeats]

Thanks for this issue, it's very valuable!

The library is mainly a port from node-ratelimiter, and looks it has the buggy things as well.

Can you make a PR for addressing that? 🙂

[bard]

The bug is actually that async-ratelimiter works with ioredis but not redis. The code in node-ratelimiter is not equivalent, it has:

      var count = parseInt(isIoRedis ? res[1][1] : res[1]);

Whereas async-ratelimiter has:

    const count = parseInt(res[1][1])

[Kikobeats]

I created a separated PR for adding the radix https://github.com/microlinkhq/async-ratelimiter/pull/20

About the errors, the current implementation is assuming you the code doesn't have errors.

Maybe a precondition before set the values could be enough?

const res = await this.db.multi(operations).exec()

    res.forEach(([error]) => {
      if (error) throw error
    })

Opened at https://github.com/microlinkhq/async-ratelimiter/pull/21

I don't think this is the best solution but at least they are not silently ignored

[bard]

Sorry, I realize now that I wrote "the bug is actually that...". Wrong on my part. There is a bug with redis/ioredis support, and it happens to cause the same error that @niftylettuce reported, but it's not necessarily the bug that @niftylettuce encountered. I will open a separate issue later.

[Kikobeats]

@niftylettuce is still a thing? I patched the parseInt addressing your first point.

About the second point, it is still not clear to me it's a bug, can you make a PR? Then we can see if tests will pass or not

[niftylettuce]

I have to test again, will let you know

[Kikobeats]

Hello, I'm going to close this issue since I think it's already fixed.

Please, feel free to comment to reopen it.