Success axios interceptors fired multiple times

[lavagri]

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]

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]

@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]

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]

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]

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]

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]

[...] 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.

[mmilardic]

Hello, since the PR has not been merged, I thought someone might find this workaround useful. It resolved the issue in my case.

The idea is to provide axios-retry with a different Axios client than the one used by other interceptors. This way we can isolate axios-retry from other interceptors and prevent it from triggering them multiple times. This ensures that other interceptors don’t see the "N effect" in case of retries.

example:
main.ts

export function createClient(config): AxiosInstance {
    const client = Axios.create(config);
    const retryClient = Axios.create(config);

    const interceptorsManager = new InterceptorsManager();

    interceptorsManager.useInterceptor(
        new RetryInterceptor(retryClient, {
            retries: 0,
            retryCondition: isNetworkOrIdempotentRequestError,
        })
    );

    interceptorsManager.useInterceptor(new CacheInterceptor());
    interceptorsManager.useInterceptor(new LoggerInterceptor());

    interceptorsManager.applyAxiosInstance(client);

    return client;
}

RetryInterceptor.ts

class RetryInterceptor  {
    constructor(
        private client: AxiosInstance,
        retry: IAxiosRetryConfig
    ) {
        axiosRetry(client, {
            retries: 0,
            retryCondition: isNetworkOrIdempotentRequestError,
            ...retry,
        });
        super();
    }

    async onRejectedResponse(error: AxiosError): Promise<AxiosResponse> {
        if (error.response) {
            return super.onFulfilledResponse(error.response);
        }

        const { config } = error;

        if (!config) {
            return Promise.reject(error);
        }

        const response = await this.client(config);

        if (response.status >= 200 && response.status < 300) {
            return super.onFulfilledResponse(response);
        }

        return super.onRejectedResponse(error);
    }
}

The RetryInterceptor wrapper class is applied just like any other interceptor. In case of an error response, it uses the isolated axios-retry client which handles the retries. If the retry request succeeds or retries are exhausted, we propagate back to the other interceptors by calling return onFulfilledResponse(response); or return super.onRejectedResponse(error);