There should be an exception in a case of "too many redirects"

[hellysmile]

Long story short

When loading http://relax-tip.de/ it redirects to http://deepskin.de/, but http://deepskin.de/ redirects back to http://relax-tip.de/.

aiohttp has built-in limit max_redirects=10, but when this limit is reached users get last redirect response and no way to figure-out that is cycle redirect, as well aiohttp follows redirects automatically and it is no expected to get 304 response.

Expected behaviour

I have an idea, when redirects limit is reached raise TooManyRedirects (requests do it)

Actual behaviour

10 redirect response returned as successful response

Steps to reproduce

import aiohttp
import asyncio


async def main():
    async with aiohttp.ClientSession() as session:
        async with session.get('http://relax-tip.de/') as response:
            import ipdb; ipdb.set_trace()
            response


loop = asyncio.get_event_loop()
loop.run_until_complete(main())

Your environment

client

If such idea makes sense, I'll do pull request to implement this feature

[asvetlov]

Google chrome gets

This page isn’t working
relax-tip.de redirected you too many times.
Try clearing your cookies.
ERR_TOO_MANY_REDIRECTS

I think the same behavior in aiohttp makes sense.

[code-cro]

Hi, i did some reading:
@hellysmile says that it should throw "TooManyRedirects" but according to wikipedia:
"The user has sent too many requests in a given amount of time. Intended for use with rate-limiting schemes."
i see problem with the end of that paragraph, because we are not doing rate-limiting here, no?

I think this kind of exception should be excpeted with 421 Misdirected Request:
https://en.wikipedia.org/wiki/List_of_HTTP_status_codes#421

Maybe even to throw:
"404 Not Found
The requested resource could not be found but may be available in the future. Subsequent requests by the client are permissible."

Because in the end the "client/agent" did not "found" the resource in the end, right?

This is problem of Recursion: https://en.wikipedia.org/wiki/Recursion
as we started basically an infinite loop, we did stop it with request recurrence number limit doe, but still this is recursion, so following some Python patterns this custom exception would be named something alike:
RequestRedirectRecursionError
Or by looking into the aiohttp code:
ClientRedirectRecursionError to be more consistent.

But how to handle it is another story :), since in @hellysmile example in "Steps to reproduce" client managed to get response on the LAST recurrence can we say we got the resource?
I think not, and that is why i would treat this exception as "404 Not Found".

Draft:
class ClientRedirectRecursionError(ClientError):
""" Recursion loop found in the request redirect cycle. """

Not sure how to handle it doe yet.

[asvetlov]

In my mind 404 Not Found means exactly a server response with 404 status code.
But in case of too many redirects (it not necessary recursive redirections, maybe the chain is just longer than max_redirects parameter in aiohttp) status code for last response is 3XX. That's why a new TooManyRedirects exception should be added.
Perhaps TooManyRedirects should be derived from aiohttp.ClientResponseError

[code-cro]

Agreed on 404 i went wrong in that direction.
So basically we talk here about two exceptions not one:

ClientRedirectRecursionError(ClientResponseError):
""" Recursion loop found in the request redirect cycle. """
What if max_redirects would be big number here lets say > 2000 would you get RecurssionError
as in python intrepeter?

ClientRedirectionTimeout(ClientResponseError):
""" Client timed out waiting for the response. """
or
ClientRedirectionMaximumReached(ClientResponseError):
""" Client has been redirected to many times without response. """
This exception in case of some small max_redirects, well that is a kind of timeout, and in this "timeout" the client did not manage to "find" response.

[asvetlov]

requests count a number of redirection limit (30 by default) and raises requests.TooManyRedirects.

I think the same behavior is good for aiohttp too (exception the limit is 10 for historical reasons, let's not touch it now).

Too high max_redirects is bad because a cost of HTTP request is much higher than cost of Python recursive call.

Redirection timeout is not what I want to have: the timeout depends on network speed and it interfere with timeout for single request.

[code-cro]

"Too high max_redirects is bad because a cost of HTTP request is much higher than cost of Python recursive call."
Yup, somebody could put max_redirects to big number by accident, for testing purposes...so what would/should happen then?
How about to limit that number to which it could be set and if somebody sets max_redirects over that number then you would raise ClientMaximumRedirectError("Too high max_redirects is bad because...") or issue a warning at least.

And in the case of reaching max_redirects:
ClientRedirectMaximumReached(ClientResponseError):
""" Client has been redirected to many times without response. """

[asvetlov]

No, it is overengineering. If somebody want to shoot the leg -- we do nothing.
Otherwise for consistency we should add such rings and bells (sane checks) to every parameter in every aiohttp call.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a [new issue] for related bugs.
If you feel like there's important points made in this discussion, please include those exceprts into that [new issue].
[new issue]: https://github.com/aio-libs/aiohttp/issues/new