Multiple setInterval

[lukiano]

Prerequisites

• I have written a descriptive issue title
• I have searched existing issues to ensure the issue has not already been raised

Issue

Hi! I was interested in this library after reading Matteo's newsletter. I've looked at the code, and some questions came to mind:

1. If you have thousands of concurrent requests that are responding throttled, it would result in thousands of setInterval being set. Have you considered using one "shared" given that the interval amount is always the same (1000)?
2. In intervalHandler, I wonder if there's a (very remote) chance of the setImmediate callback being called after another setInterval callback is executed. Then, setImmediate is called again because the buffer is not cleared. Have you considered calling _transform() within the setInterval callback and only doing this._callback() on setImmediate?

function intervalHandler (instance) {
  try {
    instance._allowedBytes = instance.bytesPerSecondFn((Date.now() - instance.startTime) / 1000, instance.bytes)

    if (
      instance._allowedBytes !== 0 &&
      instance._buffer !== null
    ) {
      const cb = this._callback;
      this._callback = (err) => setImmediate(() => cb(err));
      instance._transform();
    }
  } catch (err) {
    // Handle any errors during interval updates
    instance.emit('error', err)
  }
}

True that if setImmediate executes more than a second later, then there are bigger problems with the service, but at least this won't add to the issue.

3. Have you considered making the ThrottleStream standalone so it can be used with other frameworks?

[Uzlopak]

Thank you for your interest in this plugin.

Regarding your questions:

to 1) We had this discussion in the initial PR #1 and I didnt tackle it.
FYI: I was working for few weeks on the ThrottleStream implementation and wanted at the end to release a working product. So implementing a central setInterval was a nice to have but not a mandatory feature.

to 2) It maybe sounds strange, but I have no recollection if I tried this or not. We released about 2 weeks ago fastify-throttle but I have the feeling that I wrote this code like million years ago. LOL.

to 3) Tbh. it came to my mind. I investigated multiple implementations of throttle stream and tbh. my implementation was the only implementation using the leaky bucket algorithm in a simple way. And I really tried to reduce the whole complexity of ThrottleStream to a bare minimum, to make it easy to extend. I am totally ok to release it as a separate package.

What I think is really needed is some concept regarding ThrottleGroups. I was writing a LocalStore and RedisStore like fastify-rate-limit uses. But it was hard to implement it in a reliable manner. And the issues are manyfold:
You have to make it possible to store allowedBytes in the store, and also keep somehow the setInterval in sync between the fastify instances. It is a quiet challenging task.

Maybe I have still the code on my harddrive for my various approaches.

But at the moment we have a good concept on how to implement ThrottleGroups it will probably result in heavy modifications of the ThrottleStream implementation, resulting in a way more complex and a strongly coupled code to our fastify-throttle needs. So I assume, that compatibility with other frameworks will diminish.

Hope my babble makes sense. Looking for your feedback.

[lukiano]

Sorry for the late reply.

I've just looked up the definition of ThrottleGroups. I can't say it's something I have needed in the past, but I'm looking forward to it.
My use case was more related to grabbing the ThrottleStream and applying it to other frameworks like Koa and Express (TBH it should work in anything that runs an http.Server).

[a-a-GiTHuB-a-a]

I mean, to be fair, this is a Fastify plugin. Maybe a different package could handle a centralized http.Server throttler? I don't know, though.

[Uzlopak]

What is the concern? That I should have published a separate package?

[a-a-GiTHuB-a-a]

No, not at all! There are a looot of other throttlers out there. Surely at least one of them already handles http.Servers…