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

Add maxRedirects option #838

Closed
petrpatek opened this issue Jul 22, 2019 · 9 comments · Fixed by #914
Closed

Add maxRedirects option #838

petrpatek opened this issue Jul 22, 2019 · 9 comments · Fixed by #914
Labels
enhancement This change will extend Got features ✭ help wanted ✭

Comments

@petrpatek
Copy link

What problem are you trying to solve?

I am trying to follow more than 10 redirects.

Describe the feature

The configurable property, that will override the hardcoded 10.

@sindresorhus
Copy link
Owner

How many redirects are you trying to follow and in what crazy situation would it be more than 10?

@petrpatek
Copy link
Author

I am using it for a web-scraping and in some cases like for example on TripAdvisor, they have more redirects than ten. I think it might be connected that the request is redirected through a lot of bot check servers. In comparison, Chrome browser uses 20 redirects. I guess it is an edge use case that most of got users don't need, but on the other hand, it will be more configurable.

@sindresorhus
Copy link
Owner

Will 20 be enough for you?

@sindresorhus
Copy link
Owner

The point of the limitation is to prevent infinite loops, not valid use-cases. I would also prefer not to make it configurable as everything that is configurable adds extra overhead for the maintainers and also for users having to read the docs.

@petrpatek
Copy link
Author

20 would be great. I will create a PR for it. Thank you very much 👍

@gajus
Copy link
Contributor

gajus commented Jul 25, 2019

I would much rather have a default low, e.g. 2 redirects and maxRedirects configuration.

This is simply because 99% of websites will never make more than 1-2 redirects at a time.

We are using Got to aggregate data from thousands of websites. If one of them started to redirect in loop (e.g. broken session) and we had 50 jobs running, then 20x50 could easily result in some service disruption.

That said, we already don't use followRedirect at all for exactly this reason (even 10 is high).

@szmarczak
Copy link
Collaborator

I agree with @gajus IMHO it should be configurable and we should detect infinite loops immediately without reaching the limit.

@gajus
Copy link
Contributor

gajus commented Jul 25, 2019

and we should detect infinite loops immediately without reaching the limit.

That is unfortunately impossible. There are cases where services are going to redirect back and forth while waiting for something to happen.

@sindresorhus
Copy link
Owner

Alright. Let's leave the default at 10 and expose an option for this.

@sindresorhus sindresorhus added enhancement This change will extend Got features ✭ help wanted ✭ labels Sep 17, 2019
@sindresorhus sindresorhus changed the title Add maxRedirects option Add maxRedirects option Sep 17, 2019
@szmarczak szmarczak mentioned this issue Nov 2, 2019
4 tasks
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

Successfully merging a pull request may close this issue.

4 participants