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

Retry feature #98

Closed
sindresorhus opened this issue Sep 10, 2015 · 4 comments
Closed

Retry feature #98

sindresorhus opened this issue Sep 10, 2015 · 4 comments
Assignees
Labels
enhancement This change will extend Got features ✭ help wanted ✭
Milestone

Comments

@sindresorhus
Copy link
Owner

Networks suck. They fail all the time and are fragile. Having functionality for got to retry in case of failure would make it a lot more solid.

@vdemedes made this great module we could use a inspiration: https://github.com/vdemedes/got-retry

Question: Should it be on by default?

@floatdrop Thoughts?

Related: request/request#1300

@sindresorhus sindresorhus added enhancement This change will extend Got features ✭ help wanted ✭ labels Sep 10, 2015
@floatdrop
Copy link
Contributor

@sindresorhus I'm kind of against retry functionality in got, yet it come in handy from time to time. Here's why:

  • retry will add quite a few options - retryCount, retryDelay and etc...
  • Streams will not support this without buffering and be useless by default (if retry will be enabled by default)
  • Same as redirect, retry should be enabled only for GET and HEAD, right? Should it be configurable?

So it is kind of heavy feature and I believe, that retry should be added consciously by developer.

const retry = require('retry');
const got = require('got');

let result = await retry(() => { return got('google.com'); });

@vadimdemedes
Copy link

@floatdrop

  • Yes, more options would be added, but it is not a bad thing, is it?
  • True, but we can mention this behavior explicitly in the docs
  • retry would be enabled for all requests, because it would retry requests only on network failures, like EAI_AGAIN, ETIMEDOUT, etc.

So, to sum up, the implementation would be pretty lightweight, but the benefits are obvious. The only point is to deal with network failures, not response failures (interpreting status code, etc).

@sindresorhus
Copy link
Owner Author

I didn't mean we should implement all the retry options. I just want a option-less sane default.

The benefit of including it in core is that all consumers benefit without doing any extra work, or more importantly without having to know it's needed in the first place. Most wouldn't even be aware they should be using retry with got. If we decide to recommend it in the readme instead, all got consumers will have to do the same with their readmes, etc...

@floatdrop
Copy link
Contributor

The only point is to deal with network failures, not response failures

@vdemedes then I misunderstood issue a little - in this case streams still can be in game. Cool 🌴

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This change will extend Got features ✭ help wanted ✭
Projects
None yet
Development

No branches or pull requests

3 participants