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

Node retries not compatible with timeouts #1487

Closed
petergphillips opened this issue May 9, 2019 · 5 comments · Fixed by FAA-Aviation-Data-Portal/nfdc-airport-data#11
Closed

Comments

@petergphillips
Copy link

I'm looking at integrating superagent to make a server side call to a flaky backend API. The API either responds quickly or doesn't respond at all and needs to therefore be retried.

I'm therefore using retry(3) in combination with timeout({ deadline: 3000 }). However the timeout is overruling the retry and throwing an Aborted error which returns a 500 error to the client. From what I can tell the following is happening when the request takes too long:

  1. Superagent makes a get request to backend API
  2. GET request times out so emits an abort
  3. abort handler then rejects the request
  4. Retry logic kicks in and decides that we should retry
  5. Aborted error then causes 500 error to client
  6. Backend API is then retried
  7. If API call fails then is retried twice more otherwise finishes

Looking at the code the retry logic attempts to reset the _aborted and timedout properties, but it looks like by then it is too late as reject is called.

Is there a way I can therefore retry when the API call times out?

@petergphillips
Copy link
Author

I've investigated a bit further and it turns out that the timeout functionality is not compatible with using the promise version of superagent i.e. using then. When using end and wrapping the superagent call in a manual promise instead then it all works as expected and the request is retried after it times out.

@pushplay
Copy link

pushplay commented Jun 4, 2019

I found the same problem in version 5.0.5. Using .timeout() with .retry() worked in version 3.8.3.

@djmitche
Copy link

The issue here is that when a timeout occurs, superagent calls req.abort(), which emits an aborted message, which is handled by rejecting the promise with err.code == ABORTED. Only then does it try to reject with a timeout error -- but the promise has already rejected so that second error is silently ignored.

@djmitche
Copy link

To reproduce:

const request = require('superagent');

const main = async () => {
  const req = request('http://localhost:8000');
  req.timeout(1000);
  await req.catch(err => {
    if (err.timeout) {
      console.log('got timeout');
    }
    throw err;
  });
};

main().then(console.log, console.log);

and in another terminal, run a server that will not reply. Netcat will do:

nc -klp 8000

@niftylettuce
Copy link
Collaborator

v5.3.0 published to npm and GitHub releases page with changelog

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.

4 participants