animir / node-rate-limiter-flexible

Atomic counters and rate limiting tools. Limit resource access at any scale.
ISC License
3.07k stars 162 forks source link

Koa Middleware example: should not catch `await next()` #145

Closed mhammerc closed 2 years ago

mhammerc commented 3 years ago

Hello!

On this wiki page https://github.com/animir/node-rate-limiter-flexible/wiki/Koa-Middleware

app.use(async (ctx, next) => {
  try {
    await rateLimiter.consume(ctx.ip)
    await next()
  } catch (rejRes) {
    ctx.status = 429
    ctx.body = 'Too Many Requests'
    // Or you can throw an exception
    // ctx.throw(429, 'Too Many Requests')
  }
})

I believe we should not try/catch await next(). Only the rateLimiter.consume. If the next chain of middlewares do throw, and this middleware catch it, Koa will return a 429 Too Many Requests.

I think it may be very confusing for beginners which use this snippet and may not understand why is so.

Therefore, I propose to edit the wiki page with the following:

app.use(async (ctx, next) => {
  try {
    await rateLimiter.consume(ctx.ip)
  } catch (rejRes) {
    ctx.status = 429
    ctx.body = 'Too Many Requests'
    // Or you can throw an exception
    // ctx.throw(429, 'Too Many Requests')
    return;
  }

  await next()
})
I tested this behaviors with the following snippet. ```js const Koa = require('koa'); const app = new Koa(); app.use(async (ctx, next) => { try { await next(); } catch { ctx.body = 'catched'; } }); app.use(async ctx => { throw 'middleware2'; }) app.listen(3001); ``` I effectively get answer `catched`.
mhammerc commented 3 years ago

Oh and great job on this library by the way 👍!!!!!!

animir commented 2 years ago

@mhammerc Hey, I am glad you like it. Yes, you are right on that example fix. I updated the wiki page, thank you!