V8-archive: Add rate limiting for concurrent API HTTP requests

[bminer]

See https://github.com/directus/app/issues/1477

The main problem is that the JWT tokens may expire for long-running requests. This is especially an issue with file uploads of many files.

Also see this lib: https://github.com/bernawil/axios-concurrency

[bminer]

I'd be happy to work on a PR if desired. Thanks!

[benhaynes]

We love PRs! @rijkvanzanten — any thoughts?

[rijkvanzanten]

We should ideally implement https://github.com/directus/app/issues/1342 first. That should already tackle this specific use case and issue to an extent.

[bminer]

In the case of uploading thousands of files, the concurrent HTTP requests put an additional load on the browser.

More importantly, the /ping and /refresh routes cannot run during this time either because they are queued up by the browser just like the other HTTP requests. This is what ultimately causes the JWT token to expire... the Directus app cannot refresh it in time. Requests use the JWT token at the time the request is queued by the browser... not at the time it is dequeued.

Throttling long-running HTTP requests solves these issues and should improve application responsiveness. An important thing to note: it does not make sense to throttle all requests because you'd essentially have the same problem as now. Only long-running requests (i.e. file uploads) should be throttled.

[bminer]

Let me know what you guys think! I'd be happy to work on a PR if there's a good chance it will be accepted.

[rijkvanzanten]

Oohhh I see what you're saying now. Yeah that would be really useful to have 😄 How would it work exactly? From what it looks like, that library will queue everything after 5 requests. Can we only queue the file endpoint specifically?

[bminer]

This is just an idea... I don't think we'd use the axios-concurrency lib. Perhaps we could add a throttle function in src/index.js, and then in uploadFiles we could use it.

The following code...

      return this.axios
        .post(`${this.url}/${this.project}/files`, data, {
          headers,
          onUploadProgress
        })

...would become something like this...

      return this.throttle(() => this.axios
        .post(`${this.url}/${this.project}/files`, data, {
          headers,
          onUploadProgress
        }), 10)

The throttle function would only allow 10 concurrently running requests. When a request Promise resolves, another request would be dequeued and executed.

Probably the entire implementation of throttle is ~20 lines of code.

Thoughts?

[rijkvanzanten]

That sounds good to me! Let's call it something else than throttle though to prevent confusion with Lodash / Underscore's throttle / debounce methods, since it technically works different.

[bminer]

Will do. Good call. I'll work on a PR sometime next week if time permits.

[janbiasi]

Maybe we could also include RAF support, something like this: https://gist.github.com/janbiasi/79a3e51b29cdf819d69f50158be9d2af

[rijkvanzanten]

How would RAF help out queueing requests @janbiasi? 🙂

[janbiasi]

It doesn't help for the queuing process itself, it just helps to run certain things in the animation frame which won't affect the browser runtime itself on the technical tasking layer. It would be even better run the upload itself in a worker thread. If someone has enough time to do that I think it would be the perfect solution 😄

[rijkvanzanten]

As long as the worker thread / queueing is environment agnostic 😁 (browser js / node / react native)

[janbiasi]

With Node 11 there will be a worker package, react native doesn't have them built in but with third party modules possible 😆 but yeah wouldn't do it until react native doesn't have a great solution you're right ..

[janbiasi]

I've tested it in my PR by using a kinda concurrency limiter for the file upload - see here, feedback appreciated!

[benhaynes]

Thanks @janbiasi!

Any thoughts on this @rijkvanzanten ?

[janbiasi]

I've added a concurrency option for the API in general in the refactored version of the SDK, see: https://github.com/directus/sdk-js/blob/8568e609078ba9cd8c6e441b36e2bafabc11eab1/src/SDK.ts#L503

It's setting the concurrency at the moment statically to five (5), which should already improve the experience :)

cc @rijkvanzanten, @benhaynes @bminer