feat: allow function allowList and keyGenerator to be async/await

[DiogoMarques2003]

Checklist

• run npm run test and npm run benchmark
• tests and/or benchmarks are included
• documentation is changed or added
• commit message and code follows the Developer's Certification of Origin
  and the Code of conduct

[DiogoMarques2003]

I think isn't need add new unit test, but if is necessary pls tellme

[Uzlopak]

It seems that your code is broken.

[Uzlopak]

@DiogoMarques2003
Please provide some unit tests.

[mcollina]

Those two functions are synchronous on purpuse. Making them asynchronous and allowing I/O operations will defeat the purpose of this module.

[DiogoMarques2003]

But @mcollina i need consult the database in this functions, and the preValidation is executed after the ratelimit system

[Uzlopak]

onRequestRateLimiter is already async.

If you think that the await in front of the two potential async functions is a performance bottleneck, we could detect if one of those methods is async like this

const asyncFunctionPrototype = Object.getPrototypeOf(async function() {});

function isAsyncFunction(v) {
  return (
    typeof v === 'function' &&
    Object.getPrototypeOf(v) === asyncFunctionPrototype
  );
};

and if so have a implementation were we have await and another one were we dont await.

But again, onRequestRateLimiter is async.

[DiogoMarques2003]

@Uzlopak i need to do this on the allowList and keyGenerator functions:

can i do this databases requests in other place ?
I think no, and because of that it opens in this pull

[Uzlopak]

I was answering mcollina

The only reason to reject this PR would be for me, when the performance decreases. And awaiting a non async function decreases the performance. So if the worry of mcollina is that we decrease the perf. by unnecessary awaiting then we can check if we either have an async function and then use it with await or sync methods and dont await. But this means some refactoring to avoid simply duplicating the current function.

[DiogoMarques2003]

oh ok ok

[Fdawgs]

The only reason to reject this PR would be for me, when the performance decreases. And awaiting a non async function decreases the performance.

Got any reading material around this @Uzlopak? Is it that is awaiting a sync function causes the returned value become a resolved Promise object, and the Promise causes overhead?

[Uzlopak]

@Fdawgs

I dont think that await wraps a sync method into a Promise.

I only had this issue once about 3 years ago: A sync function was awaited. When I removed the await from the sync-function the unit test became flaky. Investigating further, it revealed a very tight race condition in the sync-function. By awaiting the sync function it resulted in a short delay, which concealed the underlying race condition. I never investigated it further.
I assume that await puts the result into the next run of the event loop, so the sync method was finished and did not show the race condition. But this is just an assumption.

[mcollina]

The problem is not about promises but rather as a conscious design decision when implementing this module: providing some limited protection against API misuse and DoS attacks.

If we need to check against a database to load some data, you are executing a database query before the check, and therefore you are likely vulnerable. In other terms: looking those things up from a database is counterproductive. This PR makes it simpler to do something insecure and I'm not convinced we should support it.

---

You can achieve what you want by adding an 'onRequest' hook before registering this module. In the 'onRequest' hook, you should add those values to the request object. I do not recommend this.

[DiogoMarques2003]

The problem is not about promises but rather as a conscious design decision when implementing this module: providing some limited protection against API misuse and DoS attacks.

If we need to check against a database to load some data, you are executing a database query before the check, and therefore you are likely vulnerable. In other terms: looking those things up from a database is counterproductive. This PR makes it simpler to do something insecure and I'm not convinced we should support it.

You can achieve what you want by adding an 'onRequest' hook before registering this module. In the 'onRequest' hook, you should add those values to the request object. I do not recommend this.

Can you show an example to add the database requests in 'onRequest' to be executed before calling the rate limit ?

[mcollina]

Something like this should work

app.addHook('onRequest', async (req) => {
  // Don't do this, it's very unsafe
  const { id, allowList } = await callMyDb()
  req.aLongPropertyName = id
  req.aLongPropertyName2 = allowList
})
fastify.register(require('@fastify/rate-limit'), ...)

[DiogoMarques2003]

The problem is not about promises but rather as a conscious design decision when implementing this module: providing some limited protection against API misuse and DoS attacks.

If we need to check against a database to load some data, you are executing a database query before the check, and therefore you are likely vulnerable. In other terms: looking those things up from a database is counterproductive. This PR makes it simpler to do something insecure and I'm not convinced we should support it.

You can achieve what you want by adding an 'onRequest' hook before registering this module. In the 'onRequest' hook, you should add those values to the request object. I do not recommend this.

The max function is async, so maybe can also be

[mcollina]

Ok, I'm sold. Let's make this possible.

[Uzlopak]

lol, I will give the approval so that you dont have to :D

[Uzlopak]

I rerun the workflow, but it seems that the tests are now flaky with the additional awaits.

[DiogoMarques2003]

Reference in ne

this is strange because on my computer they all ran correctly

[DiogoMarques2003]

this is very stranger

[Eomm]

Reference in ne

this is strange because on my computer they all ran correctly

It may depend on the node.js version.

Here are the logs of the run:
https://github.com/fastify/fastify-rate-limit/runs/7904283030?check_suite_focus=true

It seems that the namespace test is always failing.

[DiogoMarques2003]

Reference in ne

this is strange because on my computer they all ran correctly

It may depend on the node.js version.

Here are the logs of the run: https://github.com/fastify/fastify-rate-limit/runs/7904283030?check_suite_focus=true

It seems that the namespace test is always failing.

Maybe, i'm using version 16.6.0

[DiogoMarques2003]

@Eomm any soluction for this ?

[Eomm]

The CI is green https://github.com/fastify/fastify-rate-limit/actions/workflows/ci.yml

So there should be something here that has changed some function order execution

I can't debug this PR very soon

What I would do is:

1- add a check to choose if run the await or not (myFunction.constructor.name === 'AsyncFunction')
2- I would expect all tests green with this optimization

[DiogoMarques2003]

for some reason myFunction.constructor.name don't exists

[Eomm]

it was a code example, you need to use your own function keyGenerator and allowList

[DiogoMarques2003]

it was a code example, you need to use your own function keyGenerator and allowList

yes, i do this but don't work in any test

[Eomm]

Should not it be params.allowList?.constructor.name?

[Uzlopak]

@Eomm
Its a race condition. PR incoming

[Uzlopak]

#261 fixes the issues.

I think when #261 gets merged, the issues here should be also solved as the unit tests here dont use redis.

[Eomm]

Green

I think the constructor check would be good in any case

[Uzlopak]

You can actually use a prototype check

const asyncFunctionPrototype = Object.getPrototypeOf(async function() {})

function isAsyncFunction(v) {
    return (
      typeof v === 'function' &&
      Object.getPrototypeOf(v) === asyncFunctionPrototype
    )
  }

[Uzlopak]

@Eomm
What needs to be done to get this PR merged?

[Eomm]

• a quick doc note
• the isAsyncFunction check

[Uzlopak]

@Eomm
The isAsyncFunction-check is not added. I think adding that would be maybe not that smart, as a sync function returning a promise would not be detected.

[Eomm]

I'm ok landing it, there is a blocking review from @mcollina

[Uzlopak]

@mcollina

PTAL

[mcollina]

lgtm