1 patch for repository http://darcs.net:

Mon Aug  9 16:39:31 COT 2010  builes.adolfo@googlemail.com
  * Add environment variable DARCS_CONNECTION_TIMEOUT

Attachments

• add-environment-variable-darcs_connection_timeout.dpatch
• unnamed

Clarification questions:

Add environment variable DARCS_CONNECTION_TIMEOUT
-------------------------------------------------
> -             29 -> return $ Just OperationTimeout
> +             28 -> return $ Just OperationTimeout

Is this independent of your timeout patch?

Also, why is this curl only?  Perhaps you've explained this to me
already, in which case, sorry!  That said, it may actually be quite
acceptable for this to be a feature that's only supported with curl
as that's what most users are likely using Darcs with.

Also, if you're going to introduce a new environment variable, you
should ensure that it gets documented in the appropriate sections.  I
think darcs help environment is the correct place for now, but I forget
what we're supposed to do with the manual.  It'd be worth saying what
the default timeout is (perhaps using some way of avoiding duplication
of the magic number)

> +  error = set_time_out(easy);
> +  if (error != CURLE_OK){
> +    return curl_easy_strerror(error);
> +  }

> -      int error = curl_easy_getinfo(easy, CURLINFO_PRIVATE, (char **)&url_data);
> +
> +      int error = set_time_out(easy);
> +      if (error != CURLE_OK){
> +        *errorCode = error;
> +        return curl_easy_strerror(error);
> +      }
> +
> +      error = curl_easy_getinfo(easy, CURLINFO_PRIVATE, (char **)&url_data);

These seem fine

> +int set_time_out(CURL *handle)
> +{
> +  int error;
> +  long time_out;
> +  const char *stime_out;
> +
> +  stime_out = getenv("DARCS_CONNECTION_TIMEOUT");
> +  if (stime_out != NULL)
> +    time_out = atol (stime_out);

What happens to darcs if DARCS_CONNECTION_TIMEOUT is not a number?
Like "DARCS_CONNECTION_TIMEOUT=20 "

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
For a faster response, please try +44 (0)1273 64 2905.

>
> Also, why is this curl only?  Perhaps you've explained this to me
> already, in which case, sorry!  That said, it may actually be quite
> acceptable for this to be a feature that's only supported with curl
> as that's what most users are likely using Darcs with.
>
Yes, just curl.

>> +  const char *stime_out;
>> +
>> +  stime_out = getenv("DARCS_CONNECTION_TIMEOUT");
>> +  if (stime_out != NULL)
>> +    time_out = atol (stime_out);
>
> What happens to darcs if DARCS_CONNECTION_TIMEOUT is not a number?
> Like "DARCS_CONNECTION_TIMEOUT=20 "
>
probably an error, I will fix it.

--
Adolfo

1 patch for repository http://darcs.net:

Tue Aug 10 17:33:30 COT 2010  builes.adolfo@googlemail.com
  * Add environment variable DARCS_CONNECTION_TIMEOUT

Attachments

• add-environment-variable-darcs_connection_timeout.dpatch
• unnamed

Some more thoughts: maybe the 30 second value should be #defined in
hscurl.h as DEFAULT_CONNECTION_TIMEOUT.  This would be clearer and
also allow you to avoid duplication in the documentation later

Add environment variable DARCS_CONNECTION_TIMEOUT
-------------------------------------------------
> +int set_time_out(CURL *handle)
> +{
> +  int error;
> +  long time_out = 30;
> +  const char *stime_out;
> +
> +  stime_out = getenv("DARCS_CONNECTION_TIMEOUT");
> +  if (stime_out != NULL){
> +    long result = atol (stime_out);
> +    if ( result > 0 )
> +      time_out = result;
> +  }

Does this mean that parse errors are treated as 0? (which your code
would then just silently treat as the default 30s)?  Maybe set_time_out
should return an error instead so that you can notify the user that
something went wrong.

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
For a faster response, please try +44 (0)1273 64 2905.

>
> Does this mean that parse errors are treated as 0? (which your code
> would then just silently treat as the default 30s)?  Maybe set_time_out
> should return an error instead so that you can notify the user that
> something went wrong.
>
I did a trick using the pointer that we pass from URL.hs to notify
when the environment variable is not a number. Also  I mentioned about
doing the code for the timeout higher lever, but I forgot that I would
need a handle to call curl_easy_setopt.

The code I put to notify that we had an error when reading the
environment variable is not part of the libcurl codes ( and is not
taken ).

A todo thing would be to rewrite the use of curl, and put all the high
level code that we have in hscurl.c  into Haskell code.

2 patches for repository http://darcs.net:

Tue Aug 10 16:55:22 COT 2010  builes.adolfo@googlemail.com
  * Correct error code for curl operation timeout

Wed Aug 11 16:21:21 COT 2010  builes.adolfo@googlemail.com
  * Add environment variable DARCS_CONNECTION_TIMEOUT

Attachments

• correct-error-code-for-curl-operation-timeout.dpatch
• unnamed

This patch bundle (with 2 patches) was just applied to the repository http://darcs.net/.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_.html#bundle-f3ed228c5b43d6b1ae124d0e4b670b099363c596

On Wed, Aug 11, 2010 at 21:34:28 +0000, Adolfo Builes wrote:
> Tue Aug 10 16:55:22 COT 2010  builes.adolfo@googlemail.com
>   * Correct error code for curl operation timeout

Applied in an earlier run

> Wed Aug 11 16:21:21 COT 2010  builes.adolfo@googlemail.com
>   * Add environment variable DARCS_CONNECTION_TIMEOUT

I pushed this, but then I realised we could maybe do it in a
simpler way if you'd like to follow up.

Add environment variable DARCS_CONNECTION_TIMEOUT
-------------------------------------------------
> -    ce <- if not (null e)
> -          then do
> +    ce <- do
>             errorNum <- peek errorPointer
...
> -           case errorNum of
> -             6  -> return $ Just CouldNotResolveHost
> -             7  -> return $ Just CouldNotConnectToServer
> -             28 -> return $ Just OperationTimeout
> -             _  -> return Nothing
> -          else
> -           return Nothing
> +           if not (null e)
> +             then do
> +              case errorNum of
> +                6  -> return $ Just CouldNotResolveHost
> +                7  -> return $ Just CouldNotConnectToServer
> +                28 -> return $ Just OperationTimeout
> +                _  -> return Nothing
> +             else do
> +              when (errorNum == 90 ) $ debugMessage "The environment variable DARCS_CONNECTION_TIMEOUT doesn't represent a number"
> +              return Nothing

We've added a new check to for nonsense DARCS_CONNECTION_TIMEOUT
There's a tiny bit of rearranging needed because here we also need
to deal with a case where the curl_wait_url does not return an error
string.

>  
> hunk ./src/hscurl.c 279
>        CURL *easy = msg->easy_handle;
>        CURLcode result = msg->data.result;
>        struct UrlData *url_data;
> -      int error = curl_easy_getinfo(easy, CURLINFO_PRIVATE, (char **)&url_data);
> +
> +      int error = set_time_out(easy,errorCode);
> +      if (error != CURLE_OK ){
> +        *errorCode = error;
> +        return curl_easy_strerror(error);
> +      }

So my question here would be, how about extending the error array
and returning a pointer to that if error == 90?
That would allow you to have simpler code in the URL module.

Anyway, I suppose it's not a big deal and you're suggesting we could
ditch hscurl.c in favour of in Haskell using the FFI layer to talk to
curl itself.

> +int set_time_out(CURL *handle, int* errorCode)
> +{
> +  int error;
> +  long time_out = DEFAULT_CONNECTION_TIMEOUT;
> +  const char *stime_out;
> +
> +  stime_out = getenv("DARCS_CONNECTION_TIMEOUT");
> +  if (stime_out != NULL){
> +    long result = atol (stime_out);
> +    if ( result > 0 )
> +      time_out = result;
> +    else
> +      *errorCode = 90 ;
> +  }
> +
> +  error = curl_easy_setopt(handle, CURLOPT_TIMEOUT, time_out);
> +
> +  return error;
> +}

Helper function for the curl timeout setting.

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
For a faster response, please try +44 (0)1273 64 2905.

This patch bundle (with 2 patches) was just applied to the repository http://darcs.net/reviewed.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_reviewed.html#bundle-f3ed228c5b43d6b1ae124d0e4b670b099363c596