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

Coalesce concurrent access token requests #5323

Merged
merged 2 commits into from
Oct 19, 2023
Merged

Conversation

dlon
Copy link
Member

@dlon dlon commented Oct 18, 2023

Currently, multiple concurrent API calls will all send an auth request for the current account, if there is no valid access token. This PR fixes this by waiting for the in-flight request to complete if there already is one.

Fix DES-311.


This change is Reviewable

@dlon dlon force-pushed the fold-access-token-requests branch 4 times, most recently from 0391027 to 8169c7a Compare October 18, 2023 21:14
@linear
Copy link

linear bot commented Oct 19, 2023

DES-311 Bashing auth endpoints

Gregoire said:

Hej app-team!
We're getting reports from users being throttled on the app. Support posted a log that looks like this:

[2023-07-10 02:28:21.743][mullvad_api::access][DEBUG] Replacing expired access token
[2023-07-10 02:28:21.744][mullvad_api::access][DEBUG] Fetching access token for an account
[2023-07-10 02:28:21.745][mullvad_api::access][DEBUG] Replacing expired access token
[2023-07-10 02:28:21.745][mullvad_api::access][DEBUG] Fetching access token for an account
[2023-07-10 02:28:22.751][mullvad_api::access][DEBUG] Replacing expired access token
[2023-07-10 02:28:22.751][mullvad_api::access][DEBUG] Fetching access token for an account
[2023-07-10 02:28:23.148][mullvad_api::rest][ERROR] Unexpected HTTP status code 429 Too Many Requests, expected codes [200 OK]
[2023-07-10 02:28:23.148][mullvad_api::access][ERROR] Error: Failed to obtain access token
Caused by: Unexpected response status code 429 Too Many Requests - THROTTLED
[2023-07-10 02:28:23.149][mullvad_jni][ERROR] Error: Failed to get account data
Caused by: Error performing RPC with the remote API
Caused by: Unexpected response status code 429 Too Many Requests - THROTTLED

This seems to indicate that the app is sending three requests to /auth/v1/token within a second. Could that be a bug? (Concurrency issue perhaps?)

When logging in, the desktop app will retry 3 times in quick succession to get the account token. This makes it fail. There are two issues to investigate:

  • maybe bump RETRY_ACTION_INTERVAL from Duration::ZERO to 1 or 3 seconds.
  • why is the app seemingly failing to fetch the auth token in quick succession? The account seemingly is valid.

bashing-auth-99c2cac5-fbf3-48e5-aeec-081c3e769ee7.log

bashing-auth-d24c2bf0-fb5e-45b9-bb2b-84d28f9dcb9e.log

bashing-auth-19FE9F66-F8F8-4BC0-A23F-C33C75DB814E.log

bashing-auth-1a16003a-b1ff-46b5-9964-81c5bf5944c2.log

@dlon dlon marked this pull request as ready for review October 19, 2023 07:46
Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 13 of 13 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@dlon dlon force-pushed the fold-access-token-requests branch 2 times, most recently from a261f95 to 797bc18 Compare October 19, 2023 13:51
Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

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

:lgtm: 🔥

Reviewed 15 of 15 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@dlon dlon force-pushed the fold-access-token-requests branch from 797bc18 to 4e5d08e Compare October 19, 2023 14:52
@dlon dlon merged commit 437dc5b into main Oct 19, 2023
25 checks passed
@dlon dlon deleted the fold-access-token-requests branch October 19, 2023 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants