You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Yann Ylavic <yl...@gmail.com> on 2015/08/21 15:47:58 UTC
Re: svn commit: r1696105 - in /httpd/httpd/trunk: CHANGES
docs/manual/mod/mod_socache_memcache.xml modules/cache/mod_socache_memcache.c
On Sun, Aug 16, 2015 at 12:05 AM, <ja...@apache.org> wrote:
> Author: jailletc36
> Date: Sat Aug 15 22:05:08 2015
> New Revision: 1696105
>
> URL: http://svn.apache.org/r1696105
[]
> Modified: httpd/httpd/trunk/modules/cache/mod_socache_memcache.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/cache/mod_socache_memcache.c?rev=1696105&r1=1696104&r2=1696105&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/cache/mod_socache_memcache.c (original)
> +++ httpd/httpd/trunk/modules/cache/mod_socache_memcache.c Sat Aug 15 22:05:08 2015
[]
> @@ -310,6 +319,35 @@ static const ap_socache_provider_t socac
[]
> +static const char *socache_mc_set_ttl(cmd_parms *cmd, void *dummy,
> + const char *arg)
> +{
> + socache_mc_svr_cfg *sconf = ap_get_module_config(cmd->server->module_config,
> + &socache_memcache_module);
> + int i;
> +
> + i = atoi(arg);
> +
> + if ((i < 1) || (i > 1800)) {
Why a limit of 1800? Maybe the implicit INT_MAX is good enough.
Also the memcached may never close its connections by itself, or
always do, so -1 and 0 could also be interesting...
> + return apr_pstrcat(cmd->pool, cmd->cmd->name,
> + " must be a number between 1 and 1800.", NULL);
> + }
> +
> + /* apr_memcache_server_create needs a ttl in usec. */
> + sconf->ttl = i * 1000 * 1000;
sconf->ttl = apr_time_from_sec(i) ?
> +
> + return NULL;
> +}
Regards,
Yann.
Re: svn commit: r1696105 - in /httpd/httpd/trunk: CHANGES
docs/manual/mod/mod_socache_memcache.xml modules/cache/mod_socache_memcache.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Mon, Aug 24, 2015 at 11:39 PM, Christophe JAILLET
<ch...@wanadoo.fr> wrote:
> Le 24/08/2015 11:18, Yann Ylavic a écrit :
>>
>> I don't know if memcached can be configured to always close the
>> connections when done (I guess so), but since the minimal TTL is 1
>> second, we could end up reusing connections closed remotely (without
>> recovering on error) in this case.
>
> AFAIK, memcached can not be configured to close connections by itself.
No keepalive/idle timeout?
>
> Apparently all mpm are configured to ignore SIGPIPE, so apr_socket_send()
> should just return the error and the logic in apr_memcache should handle it.
> I.e.:
> ms_bad_conn(ms, conn);
> apr_memcache_disable_server(mc, ms);
>
>
> So why wouldn't we recover?
We would, but AFAICS not for the current connection (and that's suitable).
> If the connection is closed for whatever reason,
> wouldn't the server be marked as DEAD?
Yes, but so will the current request which has been sent to a sink,
should the remote have closed the idle connection (under us), with an
error returned to the socache.
The TTL aims to be synchronized with the remote idle timeout, so that
on the client side (with TTL < remote-idle-timeout) we never send
anything to a connection already (half-)closed remotely, and are sure
that any send() error comes from an interrupted connection (for
whatever reason, e.g. memcached restart).
I mean either memcached (or a fork) handles an idle timeout, or the
TTL won't help here.
For forcible closures detection, we'd need something like
ap_proxy_is_socket_connected() before reusing it (but that's also racy
and BTW should be done in apr_memcache).
>
>> So maybe our best option is to use ap_timeout_parameter_parse() to set
>> sconf->ttl, which would allow a TTL of 1 microsecond (min), and almost
>> no max (native 64bit apr_time_t)...
>>
> TTL value passed to apr_memcache_server_create is a apr_uint32_t.
> It is then passed as-is to apr_reslist_create as a apr_interval_time_t (i.e.
> apr_int64_t)
>
> So, there *IS* a max value of 2^32 usec = 4294 s ~ 3600 s = 1h
>
> Indeed, ap_timeout_parameter_parse() would be nice. This would allow TTL in
> millisecond (ms). min is for minutes.
> However having an upper limit of 4294 s (or 3600 in order to have a round
> value) still makes sense to me.
I'm fine with the upper limit too (though INT64_MAX hours would be
kind of an infinity :p ).
I was just worried about the lower one with agressive (< 1s) idle
timeouts (if that ever exists), and the compatibility with the
ineffective but legacy/working hard TTL < 1ms (600us)...
Anyway, thanks for the commit Christophe, you are probably right we
shouldn't overengineer this if not necessary.
Re: svn commit: r1696105 - in /httpd/httpd/trunk: CHANGES
docs/manual/mod/mod_socache_memcache.xml modules/cache/mod_socache_memcache.c
Posted by Christophe JAILLET <ch...@wanadoo.fr>.
Le 24/08/2015 11:18, Yann Ylavic a écrit :
> On Mon, Aug 24, 2015 at 8:17 AM, Christophe JAILLET
> <ch...@wanadoo.fr> wrote:
>> Le 21/08/2015 15:47, Yann Ylavic a écrit :
>>> On Sun, Aug 16, 2015 at 12:05 AM, <ja...@apache.org> wrote:
>>>> + if ((i < 1) || (i > 1800)) {
>>> Why a limit of 1800? Maybe the implicit INT_MAX is good enough.
>>> Also the memcached may never close its connections by itself, or
>>> always do, so -1 and 0 could also be interesting...
>> Obviously 0 should be allowed, but I don't see any reason for -1. What
>> difference would you make between the 2?
> Hm, I thought that for apr_memcache_server_create (hence
> apr_reslist_create), ttl=0 was "always close" (immediately) and -1 was
> "never close" (infinite), but that's not the case (ttl=0 means "never
> close", and -1 is invalid).
Correct.
> I don't know if memcached can be configured to always close the
> connections when done (I guess so), but since the minimal TTL is 1
> second, we could end up reusing connections closed remotely (without
> recovering on error) in this case.
AFAIK, memcached can not be configured to close connections by itself.
Apparently all mpm are configured to ignore SIGPIPE, so
apr_socket_send() should just return the error and the logic in
apr_memcache should handle it.
I.e.:
ms_bad_conn(ms, conn);
apr_memcache_disable_server(mc, ms);
So why wouldn't we recover? If the connection is closed for whatever
reason, wouldn't the server be marked as DEAD?
Following calls to apr_memcache_find_server_hash_default would try it
again after 5 s.
> So maybe our best option is to use ap_timeout_parameter_parse() to set
> sconf->ttl, which would allow a TTL of 1 microsecond (min), and almost
> no max (native 64bit apr_time_t)...
>
> Regards,
> Yann.
>
TTL value passed to apr_memcache_server_create is a apr_uint32_t.
It is then passed as-is to apr_reslist_create as a apr_interval_time_t
(i.e. apr_int64_t)
So, there *IS* a max value of 2^32 usec = 4294 s ~ 3600 s = 1h
Indeed, ap_timeout_parameter_parse() would be nice. This would allow TTL in millisecond (ms). min is for minutes.
However having an upper limit of 4294 s (or 3600 in order to have a round value) still makes sense to me.
CJ
Re: svn commit: r1696105 - in /httpd/httpd/trunk: CHANGES
docs/manual/mod/mod_socache_memcache.xml modules/cache/mod_socache_memcache.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Mon, Aug 24, 2015 at 8:17 AM, Christophe JAILLET
<ch...@wanadoo.fr> wrote:
>
> Le 21/08/2015 15:47, Yann Ylavic a écrit :
>>
>> On Sun, Aug 16, 2015 at 12:05 AM, <ja...@apache.org> wrote:
>>>
>>> + if ((i < 1) || (i > 1800)) {
>>
>> Why a limit of 1800? Maybe the implicit INT_MAX is good enough.
>> Also the memcached may never close its connections by itself, or
>> always do, so -1 and 0 could also be interesting...
>
> Obviously 0 should be allowed, but I don't see any reason for -1. What
> difference would you make between the 2?
Hm, I thought that for apr_memcache_server_create (hence
apr_reslist_create), ttl=0 was "always close" (immediately) and -1 was
"never close" (infinite), but that's not the case (ttl=0 means "never
close", and -1 is invalid).
I don't know if memcached can be configured to always close the
connections when done (I guess so), but since the minimal TTL is 1
second, we could end up reusing connections closed remotely (without
recovering on error) in this case.
So maybe our best option is to use ap_timeout_parameter_parse() to set
sconf->ttl, which would allow a TTL of 1 microsecond (min), and almost
no max (native 64bit apr_time_t)...
Regards,
Yann.
Re: svn commit: r1696105 - in /httpd/httpd/trunk: CHANGES
docs/manual/mod/mod_socache_memcache.xml modules/cache/mod_socache_memcache.c
Posted by Christophe JAILLET <ch...@wanadoo.fr>.
Hi, Yann and thanks for the review.
Le 21/08/2015 15:47, Yann Ylavic a écrit :
> On Sun, Aug 16, 2015 at 12:05 AM, <ja...@apache.org> wrote:
>> Author: jailletc36
>> Date: Sat Aug 15 22:05:08 2015
>> New Revision: 1696105
>>
>> URL: http://svn.apache.org/r1696105
> []
>> Modified: httpd/httpd/trunk/modules/cache/mod_socache_memcache.c
>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/cache/mod_socache_memcache.c?rev=1696105&r1=1696104&r2=1696105&view=diff
>> ==============================================================================
>> --- httpd/httpd/trunk/modules/cache/mod_socache_memcache.c (original)
>> +++ httpd/httpd/trunk/modules/cache/mod_socache_memcache.c Sat Aug 15 22:05:08 2015
> []
>> @@ -310,6 +319,35 @@ static const ap_socache_provider_t socac
> []
>> +static const char *socache_mc_set_ttl(cmd_parms *cmd, void *dummy,
>> + const char *arg)
>> +{
>> + socache_mc_svr_cfg *sconf = ap_get_module_config(cmd->server->module_config,
>> + &socache_memcache_module);
>> + int i;
>> +
>> + i = atoi(arg);
>> +
>> + if ((i < 1) || (i > 1800)) {
> Why a limit of 1800? Maybe the implicit INT_MAX is good enough.
> Also the memcached may never close its connections by itself, or
> always do, so -1 and 0 could also be interesting...
Obviously 0 should be allowed, but I don't see any reason for -1. What
difference would you make between the 2?
INT_MAX on a 32 bits system is 2147483647. I considered that giving the
TTL in sec was enough and "more standard". This has to be converted to sec.
So, the maximum allowed value would be 2147. 1800 (half an hour) looked
a more "logical" value for an end user. That's why I have chosen this limit.
Maybe up to 3600 (an hour) should also be allowed as it is accepted by
apr_memcache_server_create.
>
>> + return apr_pstrcat(cmd->pool, cmd->cmd->name,
>> + " must be a number between 1 and 1800.", NULL);
>> + }
>> +
>> + /* apr_memcache_server_create needs a ttl in usec. */
>> + sconf->ttl = i * 1000 * 1000;
> sconf->ttl = apr_time_from_sec(i) ?
Didn't do that because of the the different type (apr_time_t (i.e.
apr_int64_t) vs apr_uint32_t)
But using apr_time_from_sec looks good and self-document the code (i.e.
ttl in usec)
>
>> +
>> + return NULL;
>> +}
> Regards,
> Yann.
>