feat(js-sdk): support retry-after header #504
Conversation
| @@ -486,6 +488,24 @@ describe("{{appTitleCaseName}} SDK", function () { | |||
| fgaApi.check(baseConfig.storeId!, { tuple_key: tupleKey }, { retryParams: { maxRetry: 0 }}) | |||
| ).rejects.toThrow(FgaApiInternalError); | |||
| }); | |||
| it("should throw FgaApiInternalError for 500 error without Retry-After header", async () => { | |||
There was a problem hiding this comment.
Without the retry header, it should fall back to exponential backoff
| if (status === 429) { | ||
| retryDelayMs = randomTime(iterationCount, config.minWaitInMs); | ||
| } else if (status >= 500 && status !== 501) { | ||
| // For 5xx (except 501), we only retry if Retry-After was present |
There was a problem hiding this comment.
Let's change this so that we fall back to exponential backoff here too
| const secondsValue = parseInt(retryAfterValue, 10); | ||
| if (!isNaN(secondsValue)) { | ||
| const msValue = secondsValue * 1000; | ||
| if (msValue >= 1000 && msValue <= 1800000) { |
There was a problem hiding this comment.
rather than these being hard-coded, now that the go-sdk changes have been merged, you can use the following variables:
- {{maxBackoffTimeInSec}}
| if (msValue >= 1000 && msValue <= 1800000) { | |
| if (msValue > 0 && msValue <= {{retryHeaderMaxAllowableDurationInSec}}) { | |
| } |
| const now = new Date(); | ||
| const delayMs = dateValue.getTime() - now.getTime(); | ||
|
|
||
| if (delayMs >= 1000 && delayMs <= 1800000) { |
There was a problem hiding this comment.
This logic looks repeated, can you extract it to it's own function and reference it above and here?
| @@ -128,8 +128,47 @@ function isAxiosError(err: any): boolean { | |||
| function randomTime(loopCount: number, minWaitInMs: number): number { | |||
| const min = Math.ceil(2 ** loopCount * minWaitInMs); | |||
| const max = Math.ceil(2 ** (loopCount + 1) * minWaitInMs); | |||
| return Math.floor(Math.random() * (max - min) + min); //The maximum is exclusive and the minimum is inclusive | |||
| const calculatedTime = Math.floor(Math.random() * (max - min) + min); | |||
| return Math.min(calculatedTime, 120000); | |||
There was a problem hiding this comment.
| return Math.min(calculatedTime, 120000); | |
| return Math.min(calculatedTime, {{maxBackoffTimeInSec}}); |
There was a problem hiding this comment.
Defined here:
sdk-generator/config/common/config.base.json
Lines 88 to 89 in 885bdb7
| @@ -164,18 +203,40 @@ export async function attemptHttpRequest<B, R>( | |||
| throw new FgaApiAuthenticationError(err); | |||
| } else if (status === 404) { | |||
| throw new FgaApiNotFoundError(err); | |||
| } else if (status === 429 || status >= 500) { | |||
| if (iterationCount >= config.maxRetry) { | |||
| } else if (status === 429 || (status >= 500 && status !== 501)) { | |||
There was a problem hiding this comment.
Can you also make sure we are retrying on network errors?
Currently we do not retry on them: https://github.com/openfga/js-sdk/blob/8fb95c80ff57d10616c0fd60ca1dfc991ce41675/common.ts#L177
Description
Honor the retry-after header so that a client does not overwhelm the FGA server with requests.
References
Closes: openfga/js-sdk#208
Review Checklist
main