[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r1411671 - /subversion/trunk/subversion/libsvn_ra_serf/serf.c

From: Julian Foad <julianfoad_at_btopenworld.com>
Date: Tue, 20 Nov 2012 14:53:05 +0000 (GMT)

> Author: cmpilato

>
> URL: http://svn.apache.org/viewvc?rev=1411671&view=rev
> Log:
> Minor logic simplification.
>
> * subversion/libsvn_ra_serf/serf.c
> † (load_config): Simplify timeout calculations, since we know
> † † DEFAULT_HTTP_TIMEOUT is non-negative and we know the
> † † get-time-from-config path errors out on negative values.

Oops -- that logic was there to catch the fact that the default 3600 seconds goes negative in the 32-bit variable it's assigned to.

It's stupid and backwards for at least two reasons.

1) Bert says on IRC, "The timeout value in neon was used for a completely different purpose: In neon: complete request timeout, in serf: chunk received timeout. Default http timeout is 30 or 60 minutes.† A bit long for waiting for a tcp segment."

So I guess we should set the default to something more sane such as ... one minute?

2) Why store microseconds (I know it's APR's standard for time values) and allocate a 32-bit variable for it?††Either use a bigger variable or just store the number of whole seconds.

- Julian

[...]
> #define DEFAULT_HTTP_TIMEOUT 3600
> static svn_error_t *
> load_config(svn_ra_serf__session_t *session,
> @@ -268,11 +270,9 @@ load_config(svn_ra_serf__session_t *sess
> † † †††session->timeout = apr_time_from_sec(timeout);
> † †††}
> †††else
> -† † session->timeout = apr_time_from_sec(DEFAULT_HTTP_TIMEOUT);
> -
> -† if (session->timeout < 0) /* Always true for DEFAULT_HTTP_TIMEOUT */
> -† † session->timeout = apr_time_from_sec(600); /* 10 min */
> -
> +† † {
> +† † † session->timeout = apr_time_from_sec(DEFAULT_HTTP_TIMEOUT);
> +† † }
> †††SVN_ERR_ASSERT(session->timeout > 0);
Received on 2012-11-20 15:53:44 CET

This is an archived mail posted to the Subversion Dev mailing list.