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

Success axios interceptors fired multiple times #246

Open
lavagri opened this issue Oct 24, 2023 · 7 comments · May be fixed by #260
Open

Success axios interceptors fired multiple times #246

lavagri opened this issue Oct 24, 2023 · 7 comments · May be fixed by #260

Comments

@lavagri
Copy link

lavagri commented Oct 24, 2023

Recently we up our axios-retry lib version and still applied our patch to fix this bug 😅
So I decided to quickly share it, maybe someone wants to implement it in future version and just reuse our patch for that.

So here is almost full jest test we have for checking desired behavior:

const retrySpy = jest.fn();
const customInterceptorSpy = jest.fn();
const customErrorInterceptorSpy = jest.fn();

let RETRY_COUNT = 0;
let ERROR_RETURN_COUNT = 0;
const getErrorReturnCount = () => ERROR_RETURN_COUNT;

const client = axios.create({
    baseURL: 'http://localhost:8181',
    transformResponse: [
        // example of transformer, we want to know it works with it as well
        (data) => {
            try {
                return JSONBig({ storeAsString: true }).parse(data);
            } catch (err) {
                return data;
            }
        }
    ]
});

const retryOptions = {
    retries: 5,
    retryDelay: axiosRetry.exponentialDelay,
    retryCondition: (axiosError) => {
        retrySpy();
        return isAxiosRetryable(axiosError);
    }
};

// #1. Axios-retry must go first
axiosRetry(client, retryOptions);

// #2. then our custom interceptors
client.interceptors.response.use(
    (axiosResponse) => {
        // This function shouldn't be called more than once, if it does -
        // we will get undefined error (But it actually do w/o our patch❗️)
        customInterceptorSpy();
        return axiosResponse.data;
    },
    (axiosError) => {
        // This function shouldn't be called more than once, if it does -
        // we will get undefined error
        customErrorInterceptorSpy();
        return Promise.reject(new Error(axiosError.message));
    }
);

const server = http.createServer((req, res) => {
    RETRY_COUNT++;
    const shouldThrowError = RETRY_COUNT <= getErrorReturnCount();
    try {
        res.writeHead(shouldThrowError ? 504 : 200, { 'Content-Type': 'application/json' }).end(
            JSON.stringify(
                shouldThrowError
                    ? { code: 504, message: 'Request Timeout', request_id: '12345' }
                    : { code: 0, message: 'OK', request_id: '12345', data: [{ id: 'some-id' }] }
            )
        );
    } catch (e) {
        res.writeHead(e.statusCode, { 'Content-Type': 'application/json' });
        res.end(e.message);
    }
});

describe('Axios-retry', () => {
    beforeAll(async () => {
        await new Promise((resolve) => {
            server.listen(8181, () => {
                console.log('Sample HTTP Server running at 8181');
                resolve(true);
            });
            server.on('error', (err) => {
                console.error(err);
            });
        });
    });

    beforeEach(() => {
        RETRY_COUNT = 0;
        retrySpy.mockReset();
        customInterceptorSpy.mockReset();
        customErrorInterceptorSpy.mockReset();
    });

    afterAll(async () => {
        await server.close();
    });

    test('should return ok - on successful request, no retry called', async () => {
        const res = await client.get('');

        expect(res.data[0].id).toBeTruthy();

        expect(customInterceptorSpy).toBeCalledTimes(1);
        expect(customErrorInterceptorSpy).not.toBeCalled();
        expect(retrySpy).not.toBeCalled();
    });

    test('should return ok - on successful request, after several retries', async () => {
        ERROR_RETURN_COUNT = 2;
        const res = await client.get('');

        expect(res.data[0].id).toBeTruthy();

        expect(customInterceptorSpy).toBeCalledTimes(1);
        expect(customErrorInterceptorSpy).not.toBeCalled();
        expect(retrySpy).toBeCalledTimes(ERROR_RETURN_COUNT);
    });

    test('should return ok - on successful request, after all retries', async () => {
        ERROR_RETURN_COUNT = 5;
        const res = await client.get('');

        expect(res.data[0].id).toBeTruthy();

        expect(customInterceptorSpy).toBeCalledTimes(1);
        expect(customErrorInterceptorSpy).not.toBeCalled();
        expect(retrySpy).toBeCalledTimes(ERROR_RETURN_COUNT);
    }, 10_000);

    test('should return error - on failed request, after all reties exceed', async () => {
        ERROR_RETURN_COUNT = 10;

        await expect(client.get('')).rejects.toThrowError('Request failed with status code 504');

        expect(customInterceptorSpy).not.toBeCalled();
        expect(customErrorInterceptorSpy).toBeCalledTimes(1);

        expect(retrySpy).toBeCalledTimes(5);
    }, 10_000);
});

And here is our dirty patch (for CJS, if you're using ESM, consider just patch esm file same way):

diff --git a/node_modules/axios-retry/lib/cjs/index.js b/node_modules/axios-retry/lib/cjs/index.js
index 1cfaeed..bcfd989 100644
--- a/node_modules/axios-retry/lib/cjs/index.js
+++ b/node_modules/axios-retry/lib/cjs/index.js
@@ -335,11 +335,19 @@ function axiosRetry(axios, defaultOptions) {
               config.transformRequest = [function (data) {
                 return data;
               }];
+
+              var initialResInterceptors = axios.interceptors.response.handlers;
+              var [retryInterceptors] = initialResInterceptors; // we still need 1st retry interceptor
+
               onRetry(currentState.retryCount, error, config);
               return _context.abrupt("return", new Promise(function (resolve) {
                 return setTimeout(function () {
+                  // fix for not calling the interceptors for every retry-request
+                   axios.interceptors.response.handlers = [retryInterceptors];
                   return resolve(axios(config));
                 }, delay);
+              }).finally(() => {
+                 axios.interceptors.response.handlers = initialResInterceptors;
               }));
 
             case 20:

Maybe we just do not use this library in the proper way 🙃

@yutak23
Copy link
Contributor

yutak23 commented Oct 25, 2023

Since multiple axios interceptors can be registered, it seems to me that the correct behavior is that each interceptor's processing is executed when implemented as shown in the test.

cf. https://github.com/axios/axios#multiple-interceptors

@lavagri
Copy link
Author

lavagri commented Oct 30, 2023

@yutak23 Not sure I get your point.
I see the issue still, so I will try to rephrase my concern.

When I use axios with my own interceptors and eventually need to add "retry" logic, I expect to add this "retry" logic with no effect on my own old interceptor part. I want still to execute old interceptors as before - only once, not cascading as in the provided example:

original call:
----> ❌ retry.: produce new call (1)
---------> ❌ retry: produce new call (2)
---------------> ✅ retry int: succeed
---------------> (1) my int. call (?)
---------> (2) my int. call (?)
----> (3) my int. (ok)

Each time library copy my custom interceptor which results of unnecessary calling (=failed retry times) once the latest retry succeeds. If my custom interceptor transforms the response, then I gonna receive "..read the property of undefined..".

Could you confirm it's expected behavior? And if so, how then I should use my transform interceptor if I wanna add axios-retry in my project? just get rid of it?

Thanks for your time 🙂

@cb-eli
Copy link

cb-eli commented Nov 2, 2023

I'm joining @lavagri request.
I have a response interceptor that shows an error message to the user in case the server returns an error response.
When I use axios-retry the above interceptor is called four times (1st request + 3 retries) and as a result, the user sees the error message four times.

I expect my interceptor to run only once when axios-retry finishes all its retries.
Or, not run at all if one of the retries succeeded eventually.

Is it possible to apply the described behavior with the current library implementation?

@nakree1
Copy link

nakree1 commented Nov 28, 2023

I have the same issue as @lavagri described. Seems like I have to decide what to use either my custom interceptors or axios-retry interceptor only.

For me, it looks like a hidden side-effect. There is no sense in calling all interceptors multiplied by the amount of retries. I'm expecting that "axios-retry" will prevent calls to the next interceptor until it is rejected/resolved and only then it will call the next interceptor in the chain once (not multiple times) like it is described in axios docs.

It would be nice to see a config option to control such things without implementing a possible breaking change.

@mindhells
Copy link
Member

As @yutak23 mentioned, this is the expected behaviour in axios (as retries are actually new requests).
Could be interesting to have an optional config to achieve the desired behaviour described by @lavagri and suggested by @nakree1 (PRs are welcome).
On the other hand, depending on your use case, a possible workaround might be to use memoization (within the desired context) to limit the number of times a particular action is taken during the retry flow.

@xWTF
Copy link

xWTF commented Mar 21, 2024

Another thing to note is that when the interceptors transform the request / response, for example:

api.interceptors.request.use(config => {
    if (config.data) {
        config.data['some-request-sign-stuff'] = sign(config.data);

        config.data = stringify(config.data);
        config.headers['Content-Type'] = 'application/x-www-form-urlencoded';
    }
    return config;
});

api.interceptors.response.use(response => {
    // The response looks like {"code": 200, "message": "OK", "data": "your-real-data-here"} because it's wrapped in some gateway stuff
    const data = response.data;
    if (data.code) {
        if (data.code !== 200) {
            throw new ApiError(data.code, data.message);
        }
        response.data = data.data;
    } else {
        throw new Error('Unrecognized API response: ' + JSON.stringify(data));
    }
    return response;
});

The retry logic will break these inteceptors, resulting in either "trying to create some-request-sign-stuff on string object" error (when data exists and request inteceptors called twice) or Unrecognized API response error (when response inteceptor called twice). The same applies to transformRequest and transformResponse in axios config.

I'm currently just using an ugly hack to "fix" this, hope there's an "official" way to fix it (at least for the response part):

api.interceptors.request.use(config => {
    const rAny = config as any;
    if (rAny._processed_by_our_library) {
        return config;
    }
    rAny._processed_by_our_library = true;

    // ... other stuff
});

The problem here is, an interceptor is expected to be called ONCE per request call, or at least per response / per request, instead of being called multiple times on the same response (well, multiple calls on the same request object makes sense). Currently the library causes it to be called 1+N times on the same request / response object when there're N retries.

@sheerlox
Copy link

sheerlox commented Jul 3, 2024

[...] this is the expected behaviour in axios (as retries are actually new requests).

What's weird about this is that the response interceptor is called X times (the number of retries), but always with the same object, as if axios-retry was mutating the same Axios request/response object instead of actually creating new requests.

I'm using a request and a response interceptor to log the status code and the time the request took, and all printed lines contain the same information.

This doesn't feel like expected behavior, as I would at least expect the first responses to be failed requests.

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 a pull request may close this issue.

7 participants