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

Collect history of responses if redirects occur. #614

Merged
merged 5 commits into from
Nov 3, 2015

Conversation

nitori
Copy link
Contributor

@nitori nitori commented Nov 2, 2015

Hi,

Accessing the history, or the information that a redirect occured at all (except for comparing original and final URL), is something I couldn't find and was missing from the requests library.

I'm completely new to this project, so I have no idea, if what I did, is the correct way to do it or if I missed something.

Feedback appreciated.

@asvetlov
Copy link
Member

asvetlov commented Nov 2, 2015

I support the PR but tests and documentation update should be added.
Would you manage the request up to be ready for merging?

@nitori
Copy link
Contributor Author

nitori commented Nov 2, 2015

I'll ... try? (I have never really done these things)

@asvetlov
Copy link
Member

asvetlov commented Nov 2, 2015

Please do. I'll help you.
Tests first.
Add new test to https://github.com/KeepSafe/aiohttp/blob/master/tests/test_client_functional.py
Create a test which has two server handlers: one for redirection and second for returning 200 OK response.
Check for history attribute of given client response.

@nitori
Copy link
Contributor Author

nitori commented Nov 2, 2015

Oh, I didn't see your comment until after I pushed the update. :-)

@asvetlov
Copy link
Member

asvetlov commented Nov 2, 2015

Github doesn't send email notification on pushing.
So please add a comment if you need feedback.

@@ -954,6 +954,11 @@ Response object

HTTP headers of response, :class:`CIMultiDictProxy`.

.. attribute:: history

List of :class:`ClientResponse` objects of preceding requests, if there
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:class:list`` is even better

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of "List" in "List of :class:ClientResponse objects …"?

Or where exactly do you mean?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean

:class:`list` of :class:`ClientResponse` objects of preceding requests ...

@nitori
Copy link
Contributor Author

nitori commented Nov 2, 2015

Okay, I added a test.

Edit: maybe add try/finally block around the asserts to release resp/resp_redirect?

@asvetlov
Copy link
Member

asvetlov commented Nov 2, 2015

Perfect!

maybe add try/finally block around the asserts to release resp/resp_redirect?

Please do.

@nitori
Copy link
Contributor Author

nitori commented Nov 2, 2015

Done :-)

@asvetlov
Copy link
Member

asvetlov commented Nov 2, 2015

Let me sleep on it and merge tomorrow.

asvetlov added a commit that referenced this pull request Nov 3, 2015
Collect history of responses if redirects occur.
@asvetlov asvetlov merged commit 4000013 into aio-libs:master Nov 3, 2015
@popravich
Copy link
Member

Hi,
The idea is good but there is an issue -- you simply collect responses into history list and then provide it with last non-redirecting response, howerer those responses (in the history) can not be accessed in full manner -- if someone try something like he will end with exception:

yield from resp.history[0].text()

@asvetlov
Copy link
Member

asvetlov commented Nov 3, 2015

Yes, all responses except the latest are released.
What do you suggest?
Requests has the same attribute IIRC.

@popravich
Copy link
Member

I'm not quite sure what to do...
resp.read() / json() / text() coroutines use content attribute which is instance of some stream parser bound to connection.reader directly. And it all goes deeper into Connector/Connection stuff...
Need to look into this...

@asvetlov
Copy link
Member

asvetlov commented Nov 3, 2015

Good catch.
We may do better cleanup in .close()/.release()

Converted .history into readonly property by eac7028 BTW

@nitori
Copy link
Contributor Author

nitori commented Nov 3, 2015

If anything, I believe discarding the body and getting an exception when trying to access it should be the default behavior.

I assume that the common case is, that you're not even interested in the body of preceding requests (if anything, you're probably more interested in the headers or the URLs - as in my case), and in most cases, it doesn't contain any valuable information anyway.

So if you happen to be interested in the body of those requests, there should either be an option to completely load them or to somehow keep the connection opened, or to use allow_redirects=False and go through the steps yourself.

@asvetlov
Copy link
Member

asvetlov commented Nov 3, 2015

If anything, I believe discarding the body and getting an exception when trying to access it should be the default behavior.

Would you propose a patch?

@nitori
Copy link
Contributor Author

nitori commented Nov 3, 2015

Hm, I'd have to take a closer look at how things work, but I would like to give it a try.

As far as I can tell, awaiting .text() doesn't actually raise any exception but just returns an empty string on released or closed responses. Changing that would probably cause some applications, that rely on that, to break, and I don't know if it's worth to add an exemption just for redirects.

I assumed it already raises some kind of exception - so I'm not sure after all, if it's a good idea.

@asvetlov
Copy link
Member

asvetlov commented Nov 3, 2015

I don't care about .text() right now but .content should be protected from further usage.

@nitori
Copy link
Contributor Author

nitori commented Nov 3, 2015

We might as well just close the preceding responses instead of releasing them. If we do that, trying to read from .content will cause an aiohttp.errors.ServerDisconnectedError.

@asvetlov
Copy link
Member

asvetlov commented Nov 4, 2015

No, response releasing is crucial for HTTP keep-alive support.
.close() call breaks connection and closes socket but .release() allows to reuse the socket for next request.

Replacing .content with something like EmptyStream (https://github.com/KeepSafe/aiohttp/blob/master/aiohttp/streams.py#L332) but raises RuntimeError on every read call may prevent reading from history responses.

Concurrent reading from different contents (bound with the same underlying transport) make a mess as @popravich mentioned.

Honestly using just EmptyStream is an option also: redirection responses stored in history should have no HTTP BODY anyway.

Did you get my point?

@lock
Copy link

lock bot commented Oct 29, 2019

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.

@lock lock bot added the outdated label Oct 29, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants