sindresorhus / ky

🌳 Tiny & elegant JavaScript HTTP client based on the Fetch API
MIT License
13.61k stars 364 forks source link

retry and beforeRetry not triggered #607

Closed Inhoob closed 3 months ago

Inhoob commented 3 months ago
//instance.ts
export const instance = ky.create({
  prefixUrl,
  hooks: {
    afterResponse: [afterResponse],
    beforeRequest: [beforeRequest],
    beforeRetry: [
      async ({ request }) => {
        console.log('retry!!!');
        const refreshToken = getRefreshToken();
        if (refreshToken) {
          request.headers.set('RefreshToken', refreshToken);
        }
      }
    ]
  },
  retry: {
    limit: 2,
    methods: ['GET', 'PUT', 'POST', 'DELETE'],
    statusCodes: [401, 403, 500, 504]
  }
});

//auth.ts
export const fetchUserInfo = async (): Promise<UserInfo> => {
  const res = await instance.get(url.jwt).json<UserInfo>();
  return res;
};

fetchUserInfo statusCode being 401, retry and beforeRetry not triggered.. Could anyone help identify the cause of this issue? I've been trying to figure it out for several days without success, so i need your help

sholladay commented 3 months ago

Did you figure this out @Inhoob? Your code looks correct at first glance, but I'd be happy to dig in some more if you need help.

Inhoob commented 2 months ago

@sholladay

The first mistake was capitalizing method. Changing it to lowercase resolved that issue. another problem was related to the signal.throwIfAborted used in ky‘s delay. Since my environment is React Native, which doesn’t support signal.throwIfAborted, this caused an issue. I resolved it by patching the code as follows:

 if (signal.aborted) {
  return reject(signal.reason);
}
diyorbek commented 1 month ago

@sholladay I am facing the same issue. I would appreciate any help. I don't think the problem is with signal because I have a Next.js app, and everything is happening on the client side. Here's the code:

const privateAPIClient = ky.create({
  prefixUrl: BASE_URL,
  retry: { limit: 20, methods: ["get"] },
  hooks: {
    beforeRequest: [
      async (request) => {
        console.log("before request");
        if (!request.headers.get("Authorization")) {
          request.headers.set(
            "Authorization",
            `Token ${await tokenStore.getAccessToken()}`
          );
        }
      },
    ],
    beforeRetry: [
      async ({ error, request, retryCount }) => {
        console.log("before retry!!!!!");
        console.log({ error, request, retryCount, tokenStore });

        if ((error as any)?.response?.status === 401 && retryCount === 1) {
          request.headers.set(
            "Authorization",
            `Token ${await tokenStore.getAccessToken(true)}`
          );
        }

        if (retryCount === 2) await signOut();
      },
    ],
  },
  headers: {
    "Content-Type": "application/json",
  },
});

Logs inside beforeRetry hook are not printed in the console at all. I even tried putting some "console.log"s in the Ky.js file. _retry is called, but the thrown error is not caught inside the catch block. 🤷‍♂️

sholladay commented 1 month ago

There isn't enough info here for me to diagnose this.

diyorbek commented 1 month ago

Requests are made this way using react-query:

  const { data, error } = useQuery({
    queryFn: () => privateAPIClient.get(route).json<T>(),
    retry: false,
    refetchOnWindowFocus: false,
    staleTime: Infinity
  });

I have tried sending requests just by calling .get() without react-query. But the requests are still not retried when the server responds with 401.

<button
  onClick={() =>
    privateAPIClient
      .get(
        route,
        { headers: new Headers({ Authorization: "Token ABC" }) } // fake token so that server responds with 401
      )
      .json()
  }
>
  Request
</button>

Here are the request and response headers (copied from DevTools): General

Request URL:
https://$API_HOST$/page/takrorlash-while/
Request Method:
GET
Status Code:
401 Unauthorized
Remote Address:
$$$$:443
Referrer Policy:
strict-origin-when-cross-origin

Response headers

Access-Control-Allow-Origin:
*
Allow:
GET, HEAD, OPTIONS
Alt-Svc:
h3=":443"; ma=86400
Cf-Cache-Status:
DYNAMIC
Cf-Ray:
8b27f1565b7b92d4-CPH
Content-Length:
35
Content-Type:
application/json
Cross-Origin-Opener-Policy:
same-origin
Date:
Tue, 13 Aug 2024 10:15:13 GMT
Nel:
{"success_fraction":0,"report_to":"cf-nel","max_age":604800}
Referrer-Policy:
same-origin
Report-To:
{"endpoints":[{"url":"$$$$"}],"group":"cf-nel","max_age":604800}
Server:
cloudflare
Vary:
Accept, Origin
Www-Authenticate:
Token
X-Content-Type-Options:
nosniff
X-Frame-Options:
SAMEORIGIN

Request headers

:authority:
$API_HOST$
:method:
GET
:path:
/page/takrorlash-while/
:scheme:
https
Accept:
application/json
Accept-Encoding:
gzip, deflate, br, zstd
Accept-Language:
en-US,en;q=0.9,ru-RU;q=0.8,ru;q=0.7,uz;q=0.6
Authorization:
Token ABC
Cache-Control:
no-cache
Content-Type:
application/json
Origin:
http://localhost:3000
Pragma:
no-cache
Priority:
u=1, i
Referer:
http://localhost:3000/
Sec-Ch-Ua:
"Not/A)Brand";v="8", "Chromium";v="126", "Google Chrome";v="126"
Sec-Ch-Ua-Mobile:
?0
Sec-Ch-Ua-Platform:
"macOS"
Sec-Fetch-Dest:
empty
Sec-Fetch-Mode:
cors
Sec-Fetch-Site:
cross-site
User-Agent:
Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/126.0.0.0 Safari/537.36
sholladay commented 1 month ago

Thanks for the good debugging info, that was helpful.

I believe the request is not being retried because of the 401 response status. 401s are not retried by default because, generally speaking, a server will not change its decision about whether you are authenticated no matter how many times you make an unauthenticated request.

The quick and easy fix would be to add 401 to the list of allowed retry methods.

Change this:

retry: { limit: 20, methods: ["get"] },

... to this:

retry: { limit: 20, methods: ["get"], statusCodes: [401, 408, 413, 429, 500, 502, 503, 504] },

An alternative approach would be to move your logic to an afterResponse hook.

Something roughly like this:

hooks: {
    afterResponse: [
        async (request, options, response) => {
            if (response.status === 401) {
                request.headers.set(
                    "Authorization",
                    `Token ${await tokenStore.getAccessToken(true)}`
                );

                return ky(request);
            }
        }
    ]
}

The differences here are subtle. Personally, I would probably use the second approach because if the refreshed token doesn't work for some reason, I don't want any additional retries, since it's likely they would also fail. But you could, of course, control that manually in beforeRetry if you want to.

diyorbek commented 1 month ago

@sholladay, oh wow, I didn't know 401s are not retried! I just saw in the documentation that 401 is not among the default retry status codes. Do you think it's worth adding what you said in the docs? 🤔

I also think the 2nd approach is much simpler. Thanks a lot for the help!

khiet-vo commented 1 month ago

throwIfAborted is missing on reactNative that why it doesn't trigger hook BeforeRetry.

Add this to solve.