Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

App: Fix rate limit when having much relationships #22352

Merged
merged 8 commits into from May 2, 2024

Conversation

joselcvarela
Copy link
Member

@joselcvarela joselcvarela commented May 1, 2024

Scope

When there's much different relationships1 configured on the collection, we sometimes see the following error while navigating in the App:

{
  "errors": [
    {
      "message": "Too many requests, retry after 56ms.",
      "extensions": {
        "code": "REQUESTS_EXCEEDED",
        "limit": 50,
        "reset": "2024-05-01T07:28:36.286Z",
        "stack": "DirectusError: Too many requests, retry after 56ms."
      }
    }
  ]
}

The requests come from server with status 429 Too Many Requests. This is because rate limiter is set to 50 points for 1 second and the requests from App are going beyond this.

After looking into this, we saw Directus App using p-queue in order to prevent this rate limit errors.
Although, the way it's configured does not seem right as we still see issues.
The current configuration sets the intervalCap to RATE_LIMITER_POINTS - 10 and interval to RATE_LIMITER_DURATION.
This seems right, but since the duration is too long, some requests may start after hitting the rate limit on the server.

In this PR, we change the way this is configured.
To prevent the issue mentioned above, we do the math to get the smallest interval possible. In other words, we set intervalCap to 1 and the interval to RATE_LIMITER_DURATION / RATE_LIMITER_POINTS which will give the interval for 1 point.

What's changed:

  • Prevent rate limit while using Directus App when having multiple relationships

Potential Risks / Drawbacks

  • None as I foresee

Review Notes / Questions

  • N/A

Reproduction

Before After
before.mov
after.mov

Use the following database dump to reproduce the issue:
app_fix-rate-limit.sql.zip
Then navigate between items in different collections and items in collection e

Footnotes

  1. I was not able to determine the amount but it is dependant on the values configured for the rate limiter

Copy link

changeset-bot bot commented May 1, 2024

🦋 Changeset detected

Latest commit: 035c19f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@directus/app Patch
@directus/api Patch
directus Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@paescuj paescuj self-assigned this May 1, 2024
Copy link
Member

@paescuj paescuj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did some testing and it indeed seems like p-queue has some timing/coordination issues, unfortunately I couldn't really trace it down... In contrast, when using a different lib like async-sema the rate limit is not exceeded (even with full points per interval). Unfortunately, there seems to be no suitable alternative to p-queue at the moment, asnyc-sema for example has no support to drain/pause.

The changes proposed in this PR reliably prevents the rate limit from being exceeded, so I think it's an acceptable solution for now. But I'm hoping that we'll find another solution (own queue implementation?, retry, ...) with the migration of the App to the SDK.

A small trade-off with this solution, is that when placing multiple requests at the same time, those are slightly delayed by the intervals (but this shouldn't be noticeable in most cases).

I've also realized the latency measuring didn't account for the queue (delay), so I've moved this logic into the requests itself (api.ts), only starting measuring once the request queue task is actually started.

@paescuj paescuj merged commit 8e10388 into directus:main May 2, 2024
4 checks passed
@github-actions github-actions bot added this to the Next Release milestone May 2, 2024
@joselcvarela joselcvarela deleted the app/fix-rate-limit branch May 6, 2024 05:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants