SGrondin / bottleneck

Job scheduler and rate limiter, supports Clustering
MIT License
1.82k stars 75 forks source link

Exception being thrown all the time when using the light bottleneck without a connection #127

Closed richtera closed 5 years ago

richtera commented 5 years ago

Although the exceptions themselves don't cause problems, they are very annoying when trying to debug and catch other exceptions. The this._store.heartbeat is not checked against null. I think it would be better to do

return typeof ((base = this._store.heartbeat) || {}).ref === "function" ? base.ref() : void 0;

or similar. Or as in your code:

    @_queues.on "leftzero", => @_store.heartbeat?.ref?()
    @_queues.on "zero", => @_store.heartbeat?.unref?()

image

richtera commented 5 years ago

Sorry, I just moved this from the octokit/plugin-throttling.js#97 and closed the wrong one.

SGrondin commented 5 years ago

Wow this is very surprising to me. I'm shocked that it's possible and I'm shocked no one has reported it yet.

Could you please paste your octokit/plugin-throttle settings? Are you using Bottleneck directly at all or only through the plugin?

Thanks, I'll fix this ASAP.

richtera commented 5 years ago

It's a bug in bottleneck I think. Basically the coffescript doesn't do null checking on "hearbeat" which is null for a light bottleneck without a connection. this._store.heartbeat will be null, but it's trying to execute null.ref or null.unref. Ultimately the exception is caught in your event handler, but it's throwing unnecessary exceptions.

richtera commented 5 years ago

I tried both the default call from the plugin and passing bottleneck by importing it from 'bottleneck/light'. My settings just contain

{
  onRateLimit: (retryAfter, options) => {
    winston.warn(`Request quote exhausted for request ${options.method} ${options.url}`);
    if (options.request.retryCount === 0) {
      winston.info(`Retrying after ${retryAfter} seconds`);
      return true;
    }
  },
  onAbuseLimit: (retryAfter, options) => {
    winston.warn(`Abuse detected for request $(opptions.method} ${options.url})`);
  },
}
SGrondin commented 5 years ago

Thanks, I'll release a fix this weekend

SGrondin commented 5 years ago

I'm sorry this took so long. I've been busy with another project and forgot about this issue. Thank you for your patience, it's fixed in v2.19.2