www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - std.net.curl: Bad timeout defaults

reply Johannes Pfau <nospam example.com> writes:
I've been using std.net.curl lately and there's a small issue with the
default timeouts:

dataTimeout is documented as:
/// Set timeout for activity on connection.

but this is not true! timeout_ms actually is "the maximum time that you
allow the libcurl transfer operation to take". This timeout is enforced
even if there is activity. So we currently limit all CURL operations to
2 Minutes (default limit), which sucks for download managers and other
long-running operations.

We should probably use CURLOPT_LOW_SPEED_LIMIT and
CURLOPT_LOW_SPEED_TIME instead. Any comments?
Sep 14 2012
parent reply "Jonas Drewsen" <jdrewsen nospam.com> writes:
On Friday, 14 September 2012 at 13:31:50 UTC, Johannes Pfau wrote:

 I've been using std.net.curl lately and there's a small issue 
 with the
 default timeouts:

 dataTimeout is documented as:
 /// Set timeout for activity on connection.

 but this is not true! timeout_ms actually is "the maximum time 
 that you
 allow the libcurl transfer operation to take". This timeout is 
 enforced
 even if there is activity. So we currently limit all CURL 
 operations to
 2 Minutes (default limit), which sucks for download managers 
 and other
 long-running operations.

 We should probably use CURLOPT_LOW_SPEED_LIMIT and
 CURLOPT_LOW_SPEED_TIME instead. Any comments?


This is indeed not good and the comments should be corrected. 
CURLOPT_LOW_SPEED_LIMIT and CURLOPT_LOW_SPEED_TIME seems to be 
good candidates for an extra option.

Jonas
Sep 14 2012
parent reply Johannes Pfau <nospam example.com> writes:
Am Fri, 14 Sep 2012 15:54:39 +0200
schrieb "Jonas Drewsen" <jdrewsen nospam.com>:


 On Friday, 14 September 2012 at 13:31:50 UTC, Johannes Pfau wrote:

 I've been using std.net.curl lately and there's a small issue 
 with the
 default timeouts:

 dataTimeout is documented as:
 /// Set timeout for activity on connection.

 but this is not true! timeout_ms actually is "the maximum time 
 that you
 allow the libcurl transfer operation to take". This timeout is 
 enforced
 even if there is activity. So we currently limit all CURL 
 operations to
 2 Minutes (default limit), which sucks for download managers 
 and other
 long-running operations.

 We should probably use CURLOPT_LOW_SPEED_LIMIT and
 CURLOPT_LOW_SPEED_TIME instead. Any comments?


 
 This is indeed not good and the comments should be corrected. 
 CURLOPT_LOW_SPEED_LIMIT and CURLOPT_LOW_SPEED_TIME seems to be 
 good candidates for an extra option.


I think we should rather keep the comments and make dataTimeout use
CURLOPT_LOW_SPEED_LIMIT=1 and CURLOPT_LOW_SPEED_TIME. Then we could add
an extra operationTimeout option for CURLOPT_TIMEOUT.

Don't you think CURLOPT_TIMEOUT is more an operation timeout than a
dataTimeout? It already includes connect time, so I'd say we should
call it operationTimeout and not dataTimeout.
Sep 16 2012
parent reply "Jonas Drewsen" <jdrewsen nospam.com> writes:
On Sunday, 16 September 2012 at 08:44:37 UTC, Johannes Pfau wrote:

 Am Fri, 14 Sep 2012 15:54:39 +0200
 schrieb "Jonas Drewsen" <jdrewsen nospam.com>:


 On Friday, 14 September 2012 at 13:31:50 UTC, Johannes Pfau 
 wrote:

 I've been using std.net.curl lately and there's a small 
 issue with the
 default timeouts:

 dataTimeout is documented as:
 /// Set timeout for activity on connection.

 but this is not true! timeout_ms actually is "the maximum 
 time that you
 allow the libcurl transfer operation to take". This timeout 
 is enforced
 even if there is activity. So we currently limit all CURL 
 operations to
 2 Minutes (default limit), which sucks for download managers 
 and other
 long-running operations.

 We should probably use CURLOPT_LOW_SPEED_LIMIT and
 CURLOPT_LOW_SPEED_TIME instead. Any comments?


 
 This is indeed not good and the comments should be corrected. 
 CURLOPT_LOW_SPEED_LIMIT and CURLOPT_LOW_SPEED_TIME seems to be 
 good candidates for an extra option.


 I think we should rather keep the comments and make dataTimeout 
 use
 CURLOPT_LOW_SPEED_LIMIT=1 and CURLOPT_LOW_SPEED_TIME. Then we 
 could add
 an extra operationTimeout option for CURLOPT_TIMEOUT.

 Don't you think CURLOPT_TIMEOUT is more an operation timeout 
 than a
 dataTimeout? It already includes connect time, so I'd say we 
 should
 call it operationTimeout and not dataTimeout.


It probably is. And if it is acceptable that it might break some 
users code that depends on the current behavior I'm all for it. 
I'll do a pull request for it and see what the reviewers has to 
say about it :)

/Jonas
Sep 18 2012
parent reply Johannes Pfau <nospam example.com> writes:
Am Tue, 18 Sep 2012 09:16:18 +0200
schrieb "Jonas Drewsen" <jdrewsen nospam.com>:


 
 It probably is. And if it is acceptable that it might break some 
 users code that depends on the current behavior I'm all for it. 
 I'll do a pull request for it and see what the reviewers has to 
 say about it :)
 
 /Jonas


I think we can do a breaking change here, as the old code didn't work
as documented but the new code will.

I already posted a pull request here though:
https://github.com/D-Programming-Language/phobos/pull/797
Sep 18 2012
parent "Jonas Drewsen" <jdrewsen nospam.com> writes:
On Tuesday, 18 September 2012 at 07:50:51 UTC, Johannes Pfau 
wrote:

 Am Tue, 18 Sep 2012 09:16:18 +0200
 schrieb "Jonas Drewsen" <jdrewsen nospam.com>:


 
 It probably is. And if it is acceptable that it might break 
 some users code that depends on the current behavior I'm all 
 for it. I'll do a pull request for it and see what the 
 reviewers has to say about it :)
 
 /Jonas


 I think we can do a breaking change here, as the old code 
 didn't work
 as documented but the new code will.

 I already posted a pull request here though:
 https://github.com/D-Programming-Language/phobos/pull/797


Ah bugger - I did a fix for it as well.

My fix introduced a SetLowSpeedLimit(long bytesPerSec, Duration 
d) method though.

-Jonas
Sep 18 2012