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?

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

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.

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

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

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