softonic / axios-retry

Axios plugin that intercepts failed requests and retries them whenever possible
Other
1.9k stars 167 forks source link

shouldResetTimeout=false causes unexpected behaviour #150

Closed Jokero closed 1 year ago

Jokero commented 4 years ago

Documentation says:

Unless shouldResetTimeout is set, the plugin interprets the request timeout as a global value, so it is not used for each retry but for the whole request lifecycle.

My expectation: if request has timeout=7000, the whole request lifecycle should be ended after this time. If delay + passed time is greater than original timeout, we should not do a new retry.

Actual result with exponential delay:

Code

```js const http = require('http'); const axios = require('axios'); const axiosRetry = require('axios-retry'); let c = 0; const server = http.createServer((req, res) => { console.log('request #' + ++c); res.statusCode = 500; res.end(); }); server.listen(3333, () => { axiosRetry(axios, { retryDelay: axiosRetry.exponentialDelay, shouldResetTimeout: false, }); console.time('time passed'); axios.get('http://localhost:3333/ololo', { timeout: 7000, 'axios-retry': { retries: 7 } }).catch(err => { console.timeEnd('time passed') console.log('error', err.message); }); }); ```

request #1
delay 209.1421945860514 // delay for the first retry request
config.timeout 6773.857805413949 // timeout for the first retry request

request #2
delay 462.2285987643162
config.timeout 6306.629206649633

request #3
delay 919.317707196037
config.timeout 5385.311499453595

request #4
delay 1614.4518808233686
config.timeout 3767.8596186302266

request #5
delay 3726.563682257515
config.timeout 38.29593637271137

request #6
delay 6536.084639517377
config.timeout 1

time passed: 13529.878ms // <------ instead of 7 seconds, we got 13.5
error timeout of 1ms exceeded

The library calculated the latest timeout=1ms and delay=6536ms. It's obvious that there is no need to do this attempt as it will result in immediate request cancellation. I think that rejected promise with error should be returned if this expression config.timeout - lastRequestDuration - delay <= 0:

if (!shouldResetTimeout && config.timeout && currentState.lastRequestTime) {
    const lastRequestDuration = Date.now() - currentState.lastRequestTime;
    // Minimum 1ms timeout (passing 0 or less to XHR means no timeout)
    config.timeout = Math.max(config.timeout - lastRequestDuration - delay, 1);
}
Jokero commented 4 years ago

@mawrkus What do you think?

mahnunchik commented 4 years ago

+1 also shouldResetTimeout should be true by default

Shogobg commented 2 years ago

I can also confirm this unexpected behavior - just spent a couple of hours debugging my application, until I noticed the request was just not sent, due to too low (1ms) timeout.