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

Added explanation why the timeout is doubled.(#5773) #5776

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

cpyberry
Copy link

issue

#5773

@@ -1076,3 +1076,5 @@ coffee.
r = requests.get('https://github.com', timeout=None)

.. _`connect()`: https://linux.die.net/man/2/connect

When the client has IPv6 support and the server has an IPv6 DNS record (AAAA), if the IPv4 connection fails the underlying (`urllib3`_) will automatically retry using IPv6, which may lead to an effective connection timeout of twice the specified time, so take that into account when setting the connection timeout. You can see a `Stackoverflow answer <https://stackoverflow.com/questions/33046733/force-requests-to-use-ipv4-ipv6/46972341#46972341>`_ to get around the problem.
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make this far clearer and not link to stackoverflow.

Something like

Suggested change
When the client has IPv6 support and the server has an IPv6 DNS record (AAAA), if the IPv4 connection fails the underlying (`urllib3`_) will automatically retry using IPv6, which may lead to an effective connection timeout of twice the specified time, so take that into account when setting the connection timeout. You can see a `Stackoverflow answer <https://stackoverflow.com/questions/33046733/force-requests-to-use-ipv4-ipv6/46972341#46972341>`_ to get around the problem.
The connect timeout applies to each connection attempt to an IP address. If multiple addresses exist for a domain name, we can try each one. If those fail before the connection timeout is reached, urllib3 will keep trying additional IP addresses. In some cases - like IPv6 (AAAA record) - the IPv6 connection can be slower to fail.
This phenomenon can lead to an appearance of requests ignoring or multiplying the connect timeout. Based on your specific situation, you may consider extreme solutions like patching the standard library to avoid returning IPv6 records for domain names. If done poorly, this can lead to additional issues for Requests which maintainers will not help you debug.

Copy link
Author

Choose a reason for hiding this comment

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

@sigmavirus24
Thank you for your comment.
I've added a more detailed explanation than the previous commit.

Copy link
Author

Choose a reason for hiding this comment

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

Your comment was very helpful. thanks you.
I tried it on multiple translation sites and confirmed that the meaning did not change, but the grammar may be wrong

Copy link
Contributor

Choose a reason for hiding this comment

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

We have multiple volunteers who translate the documentation for us. We don't need to put it through documentation sites. Further, IPv6 is one very specific case. My revision calls out that this is possible regardless of IPv4 or IPv6.

Copy link
Author

Choose a reason for hiding this comment

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

@sigmavirus24
I corrected the part you pointed out.
Sorry for the inconvenience.

@cpyberry
Copy link
Author

@sigmavirus24

I added what the user should do and made the phenomenon stand out.

If you have time, I'd be happy if you could see it

@jakermx
Copy link

jakermx commented Apr 12, 2021

Hi,

I would like to tell what I understand on what you want to publish, and possibly, suggest a different worlds to be more accurate.

In Networking, we have a reference model set by ISO, called OSI Ref Model, it is a 7 layers reference model that, by ISO, could made any complaint system , interoperable.

Saying that, HTTP/HTTPS is a High L4 Protocol, while a connection is High L3/ Low L4 Pair...based on this....a Read Timeout, really means that once all higher security layer have been passed a sessin with a server is established, and a request has been made....without any protocol response status within the time set as timeout, it should be consider as a read timeout, the connection timeout, means that, when the orignation point request or has a established connection with an endpoint does not response within the timeout defined.

HTTP Keepalive is a way to call TCP Keealive mechanims to take care of that connection, but...as the RFC Standard is Out of this world, the endpoint commonly drip the connectn without notification...so a Relk Read Timeout...is now conerted as a Connection Timeout..

And Finallly, just to be crystal....timeout is not being double by client....it is really set by Server side response.

Cheers

@cpyberry
Copy link
Author

cpyberry commented Apr 13, 2021

I will use it as a reference.

I'm really thankful to you

@cpyberry
Copy link
Author

I made some changes so that some people could understand

@jakermx
Copy link

jakermx commented Apr 23, 2021

The IETF Standard defines a 2 hours lapse in order to start sending TCP KA Packets, obviously, it is too long, so server managers decrease that value in order to allocate the available resource properly... I.E, Netflix have a 4 minutes timeout, instead of 2 hours.... so no matter if you have enabled HTTP KA.... the server side will drop the connection on its side after 4 minutes and when the client side tries to get data on a open soclet, it will fail, cais, that socket no logr exists...BTW, NF, has a avascript code, thtat modifies the connectin time out to 2 min.

So, having the requests/urllib3 tieout parameter enabled, it wont make any diffrence, since the server might drop the soclet before the client reches its threshold

@cpyberry
Copy link
Author

Wow!
I learned for the first time that the initial value of keepalive was 2 hours.

@sigmavirus24
Copy link
Contributor

@cpyberry I don't understand the documentation you've added here any longer. It's vague, unclear, and roundabout. You've been given clear suggestions that you can simply apply and would get this PR merged. Please either update this or close it, as I will not accept these updates in their current state

@cpyberry
Copy link
Author

cpyberry commented Jul 8, 2021

@sigmavirus24
First of all, I apologize for leaving this pull request for several months.

I want to commit the changes you have suggested, but my stupid additional commits have made it an "old suggestion".

Apparently, github can only commit latest suggestions.

Is it possible to make the same proposal again?

Next time I will not do stupid things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants