requests icon indicating copy to clipboard operation
requests copied to clipboard

sets a default timeout and resolves #3070

Open grintor opened this issue 3 years ago • 5 comments

This sets a (very high) default timeout which is guaranteed not to create any breaking changes while also fixing the longstanding issue of requests possibly hanging indefinitely in any script where the dev forgot to include a timeout. I would like to urge this commit make it into the next minor release version with the possibility of lowering the timeout to 300 seconds in the next major release version.

Please see the discussion at https://github.com/psf/requests/issues/3070

grintor avatar Mar 19 '21 13:03 grintor

I'd vote for going straight to whatever timeouts browsers have as the default and skipping the intermediate step your have here, but anything will be an important improvement.

mlissner avatar Mar 19 '21 15:03 mlissner

There are - unfortunately - people who've been made to feel unsafe contributing on issue #3070 who have emailed me in the past to urge this not be changed in a minor release. This is - regardless of what you think - backwards incompatible and thus not capable of being accepted under our policies for compatibility in minor and patch releases.

sigmavirus24 avatar Mar 23 '21 12:03 sigmavirus24

That's fine. Can we do a major release for this then?

mlissner avatar Mar 23 '21 14:03 mlissner

That is unfortunate that those people felt unsafe in the discussion. I know people can be terrible sometimes, and I wish everyone could always remain civil. While I find it far-fetched that there is a request takes over 15 minutes to respond, (and the server application does not time-out the connection itself first) I don't find it impossible, so I can understand why the change is considered to be not backwards compatible. I hope that you will still make the 300ms timeout in the next major release though.

On the other hand, there is an even more minor change, which I feel certain is not a breaking one, that I would like to propose. Setting a default value for the Connect timeout, (while leaving the read timeout indefinite), and making it's value higher than the TCP connect timeout of any supported operating system, could not possibly cause a breaking change and would prevent most deadlocks.

Have a look at this image from my Ubuntu server:

image

As you can see, it timed out in 130 seconds. Now, I know what you may be thinking, "that's just the ssh application timing out, and does not reflect some hard boundary set by the OS". But have a look at this:

image

It times out because 130 seconds is the hard-limit on TCP connections in Linux (which is actually documented here)

If you perform a similar test on windows, you will find the timeout is 21 seconds (which is actually documented here)

And for macOS, the value is 60 seconds. I can't find documentation on it, but you can repeat the test from the screenshots and see.

So you see that having a connection timeout of greater than 131 seconds serves no purpose because the underlying TCP connection will have been torn down by the OS at that point.

I think that having the library waiting on a connection that can't possibly exist can be considered a bug and should be fixed in a non-major release. It will prevent deadlocks in most situations and can't possibly break anything.

I will edit my pull request for these changes.

grintor avatar Mar 23 '21 14:03 grintor

@grintor and @sigmavirus24, I'm going to move back to #3070 to try to move the conversation forward on this, since I feel like we lack direction on this issue. Maybe we can get consensus around the fix over there, and then implement it here.

mlissner avatar Mar 23 '21 18:03 mlissner