Add timeout to stream with testing #1935

Merged
merged 2 commits into from Mar 3, 2014

Projects

None yet

7 participants

Contributor
ceaess commented Feb 28, 2014

Fixes Issue #1803

@Lukasa Lukasa was assigned by sigmavirus24 Feb 28, 2014
Collaborator

Hey @Lukasa can we have some 👀 on this? I paired with @cjstapleton on this PR this morning. Let us know what you think.

@Lukasa Lukasa commented on an outdated diff Feb 28, 2014
test_requests.py
if __name__ == '__main__':
unittest.main()
+
+
Lukasa
Lukasa Feb 28, 2014 Collaborator

I don't think we need these lines.

@Lukasa Lukasa commented on an outdated diff Feb 28, 2014
test_requests.py
@@ -1170,6 +1170,15 @@ def test_max_age_invalid_str(self):
with pytest.raises(TypeError):
morsel_to_cookie(morsel)
Lukasa
Lukasa Feb 28, 2014 Collaborator

We should probably PEP8 this: can we get an extra blank line here?

Collaborator
Lukasa commented Feb 28, 2014

Thanks for this, @cjstapleton! It looks great. =) I've made a couple of tiny stylistic notes inline, but the substance of this change is perfect.

It's worth noting that this implements part of #1801. I don't think that should be a reason not to merge this, but it's worth being aware of.

Otherwise, this is good to go. When you make those changes I'll flag this as ready to merge. Thanks! 🍰

Contributor
ceaess commented Mar 1, 2014

I think I've fixed the styling issues, now @Lukasa - thanks! :) Let me know if I've missed anything.

Collaborator
Lukasa commented Mar 1, 2014

That's perfect, it looks great. I'll flag this as ready for Kenneth to merge.

Thanks again! 🍪

Collaborator

🍰 Thanks for this @cjstapleton

Owner

We'll have to make sure we communicate this to our users. This is a big change.

@kennethreitz kennethreitz merged commit d8557a2 into kennethreitz:master Mar 3, 2014

1 check passed

default Merged build finished.
Details
vlevit commented Mar 3, 2014

Perhaps, this paragraph in the API docs should also be changed

Timeouts behave slightly differently. On streaming requests, the timeout only applies to the connection attempt. On regular requests, the timeout is applied to the connection process and downloading the full body.

Please correct me if I am wrong, but the last sentence ("downloading the full body" part) is not true. It contradicts to the note in Quickstart:

timeout is not a time limit on the entire response download; rather, an exception is raised if the server has not issued a response for timeout seconds (more precisely, if no bytes have been received on the underlying socket for timeout seconds).

Collaborator
Lukasa commented Mar 3, 2014

@vlevit: Good catch, that section of the docs has been changed.

@kennethreitz: For the moment I've made sure it'll be in the changelog. If you want something more drastic I'll whip up a section of the documentation that explains timeouts in more detail. =)

Is there a release with this patch?

Collaborator

@mortoray no. And there shouldn't be a release until we fix the Proxy Authorization exposure

Collaborator
Lukasa commented Mar 12, 2014

@mortoray This patch was only made 9 days ago, and we don't have a really rapid release cadence. =)

vlevit commented Mar 12, 2014

@Lukasa Regarding docs again, I think the code example after the timeout changes explanation is no more relevant:-)

Collaborator
Lukasa commented Mar 12, 2014

It's substantially more accurate, but is also defined in slightly abstract terms. I use the phrase 'read from the socket' because a server can drip-feed data at the rate of one byte per timeout interval and avoid tripping the read timeout.

vlevit commented Mar 12, 2014

Hmm, I am speaking about this code excerpt which is still present on the master:

tarball_url = 'https://github.com/kennethreitz/requests/tarball/master'

# One second timeout for the connection attempt
# Unlimited time to download the tarball
r = requests.get(tarball_url, stream=True, timeout=1)

# One second timeout for the connection attempt
# Another full second timeout to download the tarball
r = requests.get(tarball_url, timeout=1)
Collaborator
Lukasa commented Mar 12, 2014

Ah, yes, I'll fix that up to note that it's no longer true. (I don't want to change it unconditionally because people may be upgrading to non-master versions of Requests that are still subsequent to v2.0.0.)

Collaborator
Lukasa commented Mar 12, 2014

Ugh, actually, I don't want to update that until we ship a release of this patch, so let's leave it on the pile of work 'to do'.

@seanlis seanlis referenced this pull request in houtianze/bypy Jul 19, 2014
Closed

下载速度问题 #61

Contributor

This has shipped now, I have some doc fixes in #2187

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment