onur-ozkan / nestjs-rate-limiter

Highly configurable and extensible rate limiter library
https://npmjs.com/package/nestjs-rate-limiter
MIT License
238 stars 43 forks source link

Exception thrown when rate limit exceeded #1

Closed maayoko closed 4 years ago

maayoko commented 5 years ago

Hi, I'm getting exception thrown when rate limiter is exceeded.

 /usr/src/app/node_modules/rxjs/internal/util/hostReportError.js:4
     setTimeout(function () { throw err; }, 0);
                              ^

 TypeError: You provided an invalid object where a stream was expected. You can provide an Observable, Promise, Array, or Iterable.
     at Object.exports.subscribeTo (/usr/src/app/node_modules/rxjs/internal/util/subscribeTo.js:29:15)
     at Object.subscribeToResult (/usr/src/app/node_modules/rxjs/internal/util/subscribeToResult.js:14:26)
     at MergeMapSubscriber._innerSub (/usr/src/app/node_modules/rxjs/internal/operators/mergeMap.js:82:29)
     at MergeMapSubscriber._tryNext (/usr/src/app/node_modules/rxjs/internal/operators/mergeMap.js:76:14)
     at MergeMapSubscriber._next (/usr/src/app/node_modules/rxjs/internal/operators/mergeMap.js:59:18)
     at MergeMapSubscriber.Subscriber.next (/usr/src/app/node_modules/rxjs/internal/Subscriber.js:66:18)
     at /usr/src/app/node_modules/rxjs/internal/util/subscribeToPromise.js:7:24
     at processTicksAndRejections (internal/process/task_queues.js:89:5)

I'm using:

As far as I can see, error happens in this catch block.

I would suggest either to return an observable after response has been sent or replace this with:

throw new HttpException({
                    statusCode: HttpStatus.TOO_MANY_REQUESTS,
                    error: 'Too Many Requests',
                    message: 'Rate limit exceeded.',
                }, 429);

Waiting on your response.

RyanTheAllmighty commented 5 years ago

Sorry for the delayed response, somehow I missed this notification entirely.

I'll take a look at your suggestion and get back to you :)

RyanTheAllmighty commented 5 years ago

So I've taken a look and tried both your suggestions, and I cannot get it to work. The exception is thrown and the logger picks it up, but the resulting http response is just a 500 error.

My experience with Typescript and NestJS is fairly new, so I'll keep looking into this. There's surely something I'm doing incorrectly, so I'll keep hacking away at this.

edongashi commented 5 years ago

From the docs:

Out of the box, this action is performed by a built-in global exception filter, which handles exceptions of type HttpException (and subclasses of it). When an exception is unrecognized (is neither HttpException nor a class that inherits from HttpException), the client receives the following default JSON response:

{
  "statusCode": 500,
  "message": "Internal server error"
}

I'm not sure though if this counts for interceptors after you eject to an http context.

edongashi commented 5 years ago

I think there's an actual internal server error here: Edit: That was my fault, I was playing with different config values.

[Nest] 7064   - 07/31/2019, 7:05 PM   [ExceptionsHandler] Invalid "type" option provided to RateLimiterInterceptor. Value was "undefined" +13834ms@my-app/server: Error: Invalid "type" option provided to RateLimiterInterceptor. Value was "undefined"
 at RateLimiterInterceptor.<anonymous> (node_modules\nestjs-rate-limiter\dist\rate-limiter.interceptor.js:90:27)
d30jeff commented 4 years ago

I'm having the same issue.

Getting this error message when the limit has been reached.

    setTimeout(function () { throw err; }, 0);
                             ^

TypeError: You provided 'undefined' where a stream was expected. You can provide an Observable, Promise, Array, or Iterable.

@edongashi By any chance did you manage to find a way to solve this?

edongashi commented 4 years ago

@d30jeff I'm not sure. I think after configuring and testing in swagger it showed 429 properly. I have to test it again though because I haven't used this in production yet.

maayoko commented 4 years ago

Sorry for late response. Don't remember every detail when I was debugging nestjs framework, however if you go check nestjs internal implementation for handling errors, its implemented in a way that every error that is instance of HttpException should return response with the message and status code that you provided, and if its not framework will return 500 and internal service error message (https://github.com/nestjs/nest/blob/master/packages/core/exceptions/base-exception-filter.ts#L29).

Since we are creating an instance of HttpException if (!(exception instanceof HttpException)) should evaluate to false, but somehow it evaluates to true.

Didn't have time to debug this issue any further so I handled the issue by creating my own exception filter (something similar to https://github.com/nestjs/nest/blob/master/sample/01-cats-app/src/common/filters/http-exception.filter.ts), but how you'll handle catching those errors depends on the requirements you'll have.

Hope this helps.

RyanTheAllmighty commented 4 years ago

@maayoko yeah #10 I believe will fix this. I've noticed difference in between different nestjs versions on how this is handled, as initially throwing didn't work, but newest versions work as expected.

macrosak commented 4 years ago

@RyanTheAllmighty #10 fixed the issue for us as well, but the PR is closed right now. Could you reconsider merging it and releasing a new version?

onur-ozkan commented 4 years ago

Issue has been fixed with 011d418