Re: svn commit: r1533087 - in /httpd/httpd/trunk/modules/proxy: mod_proxy.h mod_proxy_balancer.c proxy_util.c

[Jim Jagielski]

For sure the (*conn)->pool is recycled more often...

On Oct 17, 2013, at 3:52 PM, Yann Ylavic  wrote:

> On Thu, Oct 17, 2013 at 4:10 PM,  wrote:
> Author: jim
> Date: Thu Oct 17 14:10:43 2013
> New Revision: 1533087
> 
> @@ -2087,12 +2089,9 @@ PROXY_DECLARE(int) ap_proxy_acquire_conn
>  (*conn)->close  = 0;
>  (*conn)->inreslist = 0;
> 
> -if (worker->s->uds) {
> +if (*worker->s->uds_path) {
>  if ((*conn)->uds_path == NULL) {
> -apr_uri_t puri;
> -if (apr_uri_parse(worker->cp->pool, worker->s->name, &puri) == 
> APR_SUCCESS) {
> -(*conn)->uds_path = apr_pstrdup(worker->cp->pool, puri.path);
> -}
> +(*conn)->uds_path = apr_pstrdup(worker->cp->pool, 
> worker->s->uds_path);
>  }
> 
> Shouldn't that be either :
> (*conn)->uds_path = worker->s->uds_path;
> or safer :
> (*conn)->uds_path = apr_pstrdup((*conn)->pool, 
> worker->s->uds_path);
> to avoid a leak?
> 
> Regards.
> 

Re: svn commit: r1533087 - in /httpd/httpd/trunk/modules/proxy: mod_proxy.h mod_proxy_balancer.c proxy_util.c

[Yann Ylavic]

On Thu, Oct 17, 2013 at 4:10 PM,  wrote:

> Author: jim
> Date: Thu Oct 17 14:10:43 2013
> New Revision: 1533087
>
> @@ -2087,12 +2089,9 @@ PROXY_DECLARE(int) ap_proxy_acquire_conn
>  (*conn)->close  = 0;
>  (*conn)->inreslist = 0;
>
> -if (worker->s->uds) {
> +if (*worker->s->uds_path) {
>  if ((*conn)->uds_path == NULL) {
> -apr_uri_t puri;
> -if (apr_uri_parse(worker->cp->pool, worker->s->name, &puri)
> == APR_SUCCESS) {
> -(*conn)->uds_path = apr_pstrdup(worker->cp->pool,
> puri.path);
> -}
> +(*conn)->uds_path = apr_pstrdup(worker->cp->pool,
> worker->s->uds_path);
>  }
>

Shouldn't that be either :
(*conn)->uds_path = worker->s->uds_path;
or safer :
(*conn)->uds_path = apr_pstrdup((*conn)->pool,
worker->s->uds_path);
to avoid a leak?

Regards.

Re: svn commit: r1533100 - /httpd/httpd/trunk/modules/proxy/proxy_util.c

[Yann Ylavic]

You're absolutely right, I mixed everything on this one.


On Thu, Oct 17, 2013 at 7:16 PM, Jim Jagielski  wrote:

> Look at it this way: if thelen !< dlen then its
> either the same or greater. Also, thelen is basically
> the strlen of the string copied. If it is the size
> of src, then we're fine. So the check SHOULD be
>
> if ((thelen < dlen-1) || !src[thelen]) {
>
> using dlen we could blow past the actual bounds of src.
> The above is safe since we know that src is at least thelen
> chars (since that's how many we've copied)
>
> On Oct 17, 2013, at 12:43 PM, Yann Ylavic  wrote:
>
> > On Thu, Oct 17, 2013 at 6:19 PM, Jim Jagielski  wrote:
> > Need to look, but at 1st blush it looks like an off-by-1 error
> > there.
> >
> > When source length >= dlen, apr_cpystrn() ensures dst[0:dlen - 1] ==
> src[0:dlen - 1], hence off-by-1 is useless.
> >
> > Regards.
> >
> >
> > On Oct 17, 2013, at 11:33 AM, Yann Ylavic  wrote:
> >
> > >
> > > Maybe ap_proxy_strncpy() could aso have no "slow" path with this
> change :
> > >
> > > Index: modules/proxy/proxy_util.c
> > > ===
> > > --- modules/proxy/proxy_util.c(revision 1533118)
> > > +++ modules/proxy/proxy_util.c(working copy)
> > > @@ -90,7 +90,6 @@ APR_IMPLEMENT_OPTIONAL_HOOK_RUN_ALL(proxy, PROXY,
> > >  PROXY_DECLARE(apr_status_t) ap_proxy_strncpy(char *dst, const char
> *src,
> > >   apr_size_t dlen)
> > >  {
> > > -char *thenil;
> > >  apr_size_t thelen;
> > >
> > >  /* special case: really  apr_cpystrn should handle src==NULL*/
> > > @@ -98,11 +97,8 @@ PROXY_DECLARE(apr_status_t) ap_proxy_strncpy(char
> > >  *dst = '\0';
> > >  return APR_SUCCESS;
> > >  }
> > > -thenil = apr_cpystrn(dst, src, dlen);
> > > -thelen = thenil - dst;
> > > -/* Assume the typical case is smaller copying into bigger
> > > -   so we have a fast return */
> > > -if ((thelen < dlen-1) || ((strlen(src)) == thelen)) {
> > > +thelen = apr_cpystrn(dst, src, dlen) - dst;
> > > +if (thelen < dlen || !src[dlen]) {
> > >  return APR_SUCCESS;
> > >  }
> > >  /* XXX: APR_ENOSPACE would be better */
> > > [EOS]
> > >
> > > Regards,
> > > Yann.
> > >
> >
> >
>
>

Re: svn commit: r1533100 - /httpd/httpd/trunk/modules/proxy/proxy_util.c

[Jim Jagielski]

Look at it this way: if thelen !< dlen then its
either the same or greater. Also, thelen is basically
the strlen of the string copied. If it is the size
of src, then we're fine. So the check SHOULD be

if ((thelen < dlen-1) || !src[thelen]) {

using dlen we could blow past the actual bounds of src.
The above is safe since we know that src is at least thelen
chars (since that's how many we've copied)

On Oct 17, 2013, at 12:43 PM, Yann Ylavic  wrote:

> On Thu, Oct 17, 2013 at 6:19 PM, Jim Jagielski  wrote:
> Need to look, but at 1st blush it looks like an off-by-1 error
> there.
> 
> When source length >= dlen, apr_cpystrn() ensures dst[0:dlen - 1] == 
> src[0:dlen - 1], hence off-by-1 is useless.
> 
> Regards.
> 
>  
> On Oct 17, 2013, at 11:33 AM, Yann Ylavic  wrote:
> 
> >
> > Maybe ap_proxy_strncpy() could aso have no "slow" path with this change :
> >
> > Index: modules/proxy/proxy_util.c
> > ===
> > --- modules/proxy/proxy_util.c(revision 1533118)
> > +++ modules/proxy/proxy_util.c(working copy)
> > @@ -90,7 +90,6 @@ APR_IMPLEMENT_OPTIONAL_HOOK_RUN_ALL(proxy, PROXY,
> >  PROXY_DECLARE(apr_status_t) ap_proxy_strncpy(char *dst, const char *src,
> >   apr_size_t dlen)
> >  {
> > -char *thenil;
> >  apr_size_t thelen;
> >
> >  /* special case: really  apr_cpystrn should handle src==NULL*/
> > @@ -98,11 +97,8 @@ PROXY_DECLARE(apr_status_t) ap_proxy_strncpy(char
> >  *dst = '\0';
> >  return APR_SUCCESS;
> >  }
> > -thenil = apr_cpystrn(dst, src, dlen);
> > -thelen = thenil - dst;
> > -/* Assume the typical case is smaller copying into bigger
> > -   so we have a fast return */
> > -if ((thelen < dlen-1) || ((strlen(src)) == thelen)) {
> > +thelen = apr_cpystrn(dst, src, dlen) - dst;
> > +if (thelen < dlen || !src[dlen]) {
> >  return APR_SUCCESS;
> >  }
> >  /* XXX: APR_ENOSPACE would be better */
> > [EOS]
> >
> > Regards,
> > Yann.
> >
> 
> 

Re: svn commit: r1533100 - /httpd/httpd/trunk/modules/proxy/proxy_util.c

[Yann Ylavic]

On Thu, Oct 17, 2013 at 6:43 PM, Yann Ylavic  wrote:

> On Thu, Oct 17, 2013 at 6:19 PM, Jim Jagielski  wrote:
>
>> Need to look, but at 1st blush it looks like an
>> off-by-1 error
>> there.
>>
>
> When source length >= dlen, apr_cpystrn() ensures dst[0:dlen - 1] ==
> src[0:dlen - 1], hence off-by-1 is useless.
>

Oups sorry, my bad, 
I misread apr_cpystrn(), the 
off-by-1 is
needed and ((thelen < dlen-1) || !src[dlen - 1]) is the correct test.
Yet the underflow when dlen is 0 is not very nice, maybe that could be
checked before calling apr_cpystrn() and turned to an error.


> Regards.
>
>

Re: svn commit: r1533100 - /httpd/httpd/trunk/modules/proxy/proxy_util.c

[Yann Ylavic]

On Thu, Oct 17, 2013 at 6:19 PM, Jim Jagielski  wrote:

> Need to look, but at 1st blush it looks like an
> 
> off-by-1 error
> there.
>

When source length >= dlen, apr_cpystrn() ensures dst[0:dlen - 1] ==
src[0:dlen - 1], hence off-by-1 is useless.

Regards.



> On Oct 17, 2013, at 11:33 AM, Yann Ylavic  wrote:
>
> >
> > Maybe ap_proxy_strncpy() could aso have no "slow" path with this change :
> >
> > Index: modules/proxy/proxy_util.c
> > ===
> > --- modules/proxy/proxy_util.c(revision 1533118)
> > +++ modules/proxy/proxy_util.c(working copy)
> > @@ -90,7 +90,6 @@ APR_IMPLEMENT_OPTIONAL_HOOK_RUN_ALL(proxy, PROXY,
> >  PROXY_DECLARE(apr_status_t) ap_proxy_strncpy(char *dst, const char *src,
> >   apr_size_t dlen)
> >  {
> > -char *thenil;
> >  apr_size_t thelen;
> >
> >  /* special case: really  apr_cpystrn should handle src==NULL*/
> > @@ -98,11 +97,8 @@ PROXY_DECLARE(apr_status_t) ap_proxy_strncpy(char
> >  *dst = '\0';
> >  return APR_SUCCESS;
> >  }
> > -thenil = apr_cpystrn(dst, src, dlen);
> > -thelen = thenil - dst;
> > -/* Assume the typical case is smaller copying into bigger
> > -   so we have a fast return */
> > -if ((thelen < dlen-1) || ((strlen(src)) == thelen)) {
> > +thelen = apr_cpystrn(dst, src, dlen) - dst;
> > +if (thelen < dlen || !src[dlen]) {
> >  return APR_SUCCESS;
> >  }
> >  /* XXX: APR_ENOSPACE would be better */
> > [EOS]
> >
> > Regards,
> > Yann.
> >
>
>

Re: svn commit: r1533087 - in /httpd/httpd/trunk/modules/proxy: mod_proxy.h mod_proxy_balancer.c proxy_util.c

[Jim Jagielski]

That's why I chose what I did... It's only needed once and
the pool is around. But yeah, it may be better.

On Oct 17, 2013, at 12:25 PM, Yann Ylavic  wrote:

> On Thu, Oct 17, 2013 at 6:21 PM, Jim Jagielski  wrote:
> This is uglier than creating a subpool... :)
> 
> Possibly ;) but with the pool being destroyed then...
> 
> Regards.
> 

Re: svn commit: r1533087 - in /httpd/httpd/trunk/modules/proxy: mod_proxy.h mod_proxy_balancer.c proxy_util.c

[Yann Ylavic]

On Thu, Oct 17, 2013 at 6:25 PM, Yann Ylavic  wrote:

> On Thu, Oct 17, 2013 at 6:21 PM, Jim Jagielski  wrote:
>
>> This is uglier than creating a subpool... :)
>>
>
> Possibly ;) but with the pool being destroyed then...
>

Something like :

PROXY_DECLARE(char *) ap_proxy_worker_name(apr_pool_t **p,
   proxy_worker *worker)
{
int rv;
apr_uri_t uri;
if (!(*worker->s->uds_path)) {
return worker->s->name;
}
if (!*p) {
/* ugly */
apr_pool_create(p, ap_server_conf->process->pool);
if (!*p) {
/* something is better than nothing :) */
return worker->s->name;
}
}
return apr_pstrcat(*p, "unix:", worker->s->uds_path, "|",
worker->s->name, NULL);
}

so
that 
the caller caller can :
apr_poo_t *p = NULL;
char *s = ap_proxy_worker_name(&p, worker);
// play with s
if (p) {
apr_pool_destroy(p);
}


>
> Regards.
>
>

Re: svn commit: r1533087 - in /httpd/httpd/trunk/modules/proxy: mod_proxy.h mod_proxy_balancer.c proxy_util.c

[Yann Ylavic]

On Thu, Oct 17, 2013 at 6:21 PM, Jim Jagielski  wrote:

> This is uglier than creating a subpool... :)
>

Possibly ;) but with the pool being destroyed then...

Regards.

Re: svn commit: r1533087 - in /httpd/httpd/trunk/modules/proxy: mod_proxy.h mod_proxy_balancer.c proxy_util.c

[Jim Jagielski]

This is uglier than creating a subpool... :)

On Oct 17, 2013, at 12:10 PM, Yann Ylavic  wrote:

> Maybe using the following macros could help to log worker's (full) name when 
> no pool is available for ap_proxy_worker_name().
> 
> Regards,
> 
> Index: modules/proxy/mod_proxy.h
> ===
> --- modules/proxy/mod_proxy.h(revision 1533127)
> +++ modules/proxy/mod_proxy.h(working copy)
> @@ -328,6 +328,11 @@ do { \
>  (w)->s->io_buffer_size_set   = (c)->io_buffer_size_set;\
>  } while (0)
>  
> +#define PROXY_WORKER_FMT "%s%s%s%s"
> +#define PROXY_WORKER_ARG(w) \
> +*(w)->s->uds_path ? "unix" : "", (w)->s->uds_path, \
> +*(w)->s->uds_path ?"|" : "", (w)->s->name
> +
>  /* use 2 hashes */
>  typedef struct {
>  unsigned int def;
> Index: modules/proxy/mod_proxy.c
> ===
> --- modules/proxy/mod_proxy.c(revision 1533127)
> +++ modules/proxy/mod_proxy.c(working copy)
> @@ -1548,15 +1548,15 @@ static const char *
>  } else {
>  reuse = 1;
>  ap_log_error(APLOG_MARK, APLOG_INFO, 0, cmd->server, 
> APLOGNO(01145)
> - "Sharing worker '%s' instead of creating new worker 
> '%s'",
> - ap_proxy_worker_name(cmd->pool, worker), new->real);
> + "Sharing worker '"PROXY_WORKER_FMT"' instead of 
> creating new worker '%s'",
> + PROXY_WORKER_ARG(worker), new->real);
>  }
>  
>  for (i = 0; i < arr->nelts; i++) {
>  if (reuse) {
>  ap_log_error(APLOG_MARK, APLOG_WARNING, 0, cmd->server, 
> APLOGNO(01146)
> - "Ignoring parameter '%s=%s' for worker '%s' 
> because of worker sharing",
> - elts[i].key, elts[i].val, 
> ap_proxy_worker_name(cmd->pool, worker));
> + "Ignoring parameter '%s=%s' for worker 
> '"PROXY_WORKER_FMT"' because of worker sharing",
> + elts[i].key, elts[i].val, 
> PROXY_WORKER_ARG(worker));
>  } else {
>  const char *err = set_worker_param(cmd->pool, worker, 
> elts[i].key,
> elts[i].val);
> @@ -2021,14 +2021,14 @@ static const char *add_member(cmd_parms *cmd, void
>  if ((err = ap_proxy_define_worker(cmd->pool, &worker, balancer, 
> conf, name, 0)) != NULL)
>  return apr_pstrcat(cmd->temp_pool, "BalancerMember ", err, NULL);
>  ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, cmd->server, APLOGNO(01148)
> - "Defined worker '%s' for balancer '%s'",
> - ap_proxy_worker_name(cmd->pool, worker), 
> balancer->s->name);
> + "Defined worker '"PROXY_WORKER_FMT"' for balancer '%s'",
> + PROXY_WORKER_ARG(worker), balancer->s->name);
>  PROXY_COPY_CONF_PARAMS(worker, conf);
>  } else {
>  reuse = 1;
>  ap_log_error(APLOG_MARK, APLOG_INFO, 0, cmd->server, APLOGNO(01149)
> - "Sharing worker '%s' instead of creating new worker 
> '%s'",
> - ap_proxy_worker_name(cmd->pool, worker), name);
> + "Sharing worker '"PROXY_WORKER_FMT"' instead of 
> creating new worker '%s'",
> + PROXY_WORKER_ARG(worker), name);
>  }
>  
>  arr = apr_table_elts(params);
> @@ -2036,8 +2036,8 @@ static const char *add_member(cmd_parms *cmd, void
>  for (i = 0; i < arr->nelts; i++) {
>  if (reuse) {
>  ap_log_error(APLOG_MARK, APLOG_WARNING, 0, cmd->server, 
> APLOGNO(01150)
> - "Ignoring parameter '%s=%s' for worker '%s' because 
> of worker sharing",
> - elts[i].key, elts[i].val, 
> ap_proxy_worker_name(cmd->pool, worker));
> + "Ignoring parameter '%s=%s' for worker 
> '"PROXY_WORKER_FMT"' because of worker sharing",
> + elts[i].key, elts[i].val, PROXY_WORKER_ARG(worker));
>  } else {
>  err = set_worker_param(cmd->pool, worker, elts[i].key,
> elts[i].val);
> Index: modules/proxy/mod_proxy_balancer.c
> ===
> --- modules/proxy/mod_proxy_balancer.c(revision 1533127)
> +++ modules/proxy/mod_proxy_balancer.c(working copy)
> @@ -118,8 +118,8 @@ static void init_balancer_members(apr_pool_t *p, s
>  int worker_is_initialized;
>  proxy_worker *worker = *workers;
>  ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, APLOGNO(01158)
> - "Looking at %s -> %s initialized?", balancer->s->name,
> - ap_proxy_worker_name(p, worker));
> + "Looking at "PROXY_WORKER_FMT" -> %s initializ

Re: svn commit: r1533100 - /httpd/httpd/trunk/modules/proxy/proxy_util.c

[Jim Jagielski]

Need to look, but at 1st blush it looks like an off-by-1 error
there.
On Oct 17, 2013, at 11:33 AM, Yann Ylavic  wrote:

> 
> Maybe ap_proxy_strncpy() could aso have no "slow" path with this change :
> 
> Index: modules/proxy/proxy_util.c
> ===
> --- modules/proxy/proxy_util.c(revision 1533118)
> +++ modules/proxy/proxy_util.c(working copy)
> @@ -90,7 +90,6 @@ APR_IMPLEMENT_OPTIONAL_HOOK_RUN_ALL(proxy, PROXY,
>  PROXY_DECLARE(apr_status_t) ap_proxy_strncpy(char *dst, const char *src,
>   apr_size_t dlen)
>  {
> -char *thenil;
>  apr_size_t thelen;
>  
>  /* special case: really  apr_cpystrn should handle src==NULL*/
> @@ -98,11 +97,8 @@ PROXY_DECLARE(apr_status_t) ap_proxy_strncpy(char
>  *dst = '\0';
>  return APR_SUCCESS;
>  }
> -thenil = apr_cpystrn(dst, src, dlen);
> -thelen = thenil - dst;
> -/* Assume the typical case is smaller copying into bigger
> -   so we have a fast return */
> -if ((thelen < dlen-1) || ((strlen(src)) == thelen)) {
> +thelen = apr_cpystrn(dst, src, dlen) - dst;
> +if (thelen < dlen || !src[dlen]) {
>  return APR_SUCCESS;
>  }
>  /* XXX: APR_ENOSPACE would be better */
> [EOS]
> 
> Regards,
> Yann.
> 

Re: svn commit: r1533100 - /httpd/httpd/trunk/modules/proxy/proxy_util.c

[Yann Ylavic]

On Thu, Oct 17, 2013 at 5:02 PM,  wrote:

> Author: jim
> Date: Thu Oct 17 15:02:04 2013
> New Revision: 1533100
>
> URL: http://svn.apache.org/r1533100
> Log:
> ap_proxy_strncpy should correctly handle src being NULL.
> Actually, apr_cpystrn() should as well...
>
> Modified:
> httpd/httpd/trunk/modules/proxy/proxy_util.c
>
> Modified: httpd/httpd/trunk/modules/proxy/proxy_util.c
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/proxy_util.c?rev=1533100&r1=1533099&r2=1533100&view=diff
>
> ==
> --- httpd/httpd/trunk/modules/proxy/proxy_util.c (original)
> +++ httpd/httpd/trunk/modules/proxy/proxy_util.c Thu Oct 17 15:02:04 2013
> @@ -93,6 +93,11 @@ PROXY_DECLARE(apr_status_t) ap_proxy_str
>  char *thenil;
>  apr_size_t thelen;
>
> +/* special case: really  apr_cpystrn should handle src==NULL*/
> +if (!src && dlen) {
> +*dst = '\0';
> +return APR_SUCCESS;
> +}
>  thenil = apr_cpystrn(dst, src, dlen);
>  thelen = thenil - dst;
>  /* Assume the typical case is smaller copying into bigger
>
>
>
Maybe ap_proxy_strncpy() could aso have no "slow" path with this change :

Index: modules/proxy/proxy_util.c
===
--- modules/proxy/proxy_util.c(revision 1533118)
+++ modules/proxy/proxy_util.c(working copy)
@@ -90,7 +90,6 @@ APR_IMPLEMENT_OPTIONAL_HOOK_RUN_ALL(proxy, PROXY,
 PROXY_DECLARE(apr_status_t) ap_proxy_strncpy(char *dst, const char *src,
  apr_size_t dlen)
 {
-char *thenil;
 apr_size_t thelen;

 /* special case: really  apr_cpystrn should handle src==NULL*/
@@ -98,11 +97,8 @@ PROXY_DECLARE(apr_status_t) ap_proxy_strncpy(char
 *dst = '\0';
 return APR_SUCCESS;
 }
-thenil = apr_cpystrn(dst, src, dlen);
-thelen = thenil - dst;
-/* Assume the typical case is smaller copying into bigger
-   so we have a fast return */
-if ((thelen < dlen-1) || ((strlen(src)) == thelen)) {
+thelen = apr_cpystrn(dst, src, dlen) - dst;
+if (thelen < dlen || !src[dlen]) {
 return APR_SUCCESS;
 }
 /* XXX: APR_ENOSPACE would be better */
[EOS]

Regards,
Yann.

Re: mod_proxy, oooled backend connections and the keep-alive race condition

[Yann Ylavic]

On Thu, Oct 17, 2013 at 11:36 AM, Thomas Eckert  wrote:

> > Hence,
> 
> why cannot mod_proxy_http prefetch the client's body *before* trying
> anything with the backend connection, and only if it's all good, connect
> (or reuse a connection to) the
> > backend and then start forwarding the data immediatly (flushing the
> prefetched ones) ?
>
> Now I'm confused. Are we talking about flushing everything immediately
> from client to backend (that's what I understood from your first patch
> proposal in this thread) or are we talking about pre fetching the whole
> request body from the client and then passing it on to the backend as a
> whole ?
>
>
Sorry, I really should not have proposed 2 patches in the same thread
without being clear about their different purposes.

I first thought flushing everything received from the client to the backend
immediatly, and disabling *retries* in the 16K-prefetch, would address the
issue (my first patch, "httpd-trunk-mod_proxy_http_flushall.patch"). But as
you said in a previous message, that did not address the case where the
client and/or an input filter retain the very first body bytes, and
mod_proxy_http blocks, and the backend timeouts...

Hence the second patch ("httpd-trunk-mod_proxy_http_prefetch.patch"), which
does prefetch *before* establishing/checking the backend's connection, so
to minimize (to the minimum) the future delay between that and the first
bytes sent to the backend (hence the chances for it to timeout). The
prefetch is still up to 16K (same as existing), and not the "whole" body of
course.

Then, when it's time to forward (what is available from) the client's
request, the patched mod_proxy_http will establish or check+reuse a backend
connection and always flush immediatly what it has  already (headers +
prefetch, see stream_reqbody_* functions).

For the remaining request's body chunks (if any), I intentionally did not
change the current behaviour of letting the output filters choose when to
flush, this is not the keep-alive timeout anymore, so it becomes out of the
issue's scope. But I'm definitively +1 to flush-all here too (optionaly),
since mod_proxy may retain small chunks and can then face another backend
timeout later! That's where patch1 meets patch2, and comes patch3
("httpd-trunk-mod_proxy_http_prefetch+flushall.patch", attached here).

There is still the potential (minimal) race condition, the backend could
have closed in the (short) meantime, that's indeed *not* addressed by my
patch(es).


>
> The problem I mentioned in the first message is about treating the backend
> connection in an error prone way. By not ensuring the connection is still
> valid - e.g. as seen by the peer - we risk running into this problem no
> matter what we do elsewhere. So I really think the proper way to handle
> this is to reopen the connection if necessary - be this with a filter or
> integrated into the connection handling itself - and only fail after we
> tried at least once.
>

IMHO this is hardly feasible for non-idempotent (and no body) requests
(without disabling connection reuse, or using "proxy-initial-not-pooled" to
let the client choose), even if mod_proxy takes care of not sending the
full request in one time to be able to detect connection problems before
it's too late (to retry), there is still a possibility that it won't know
before sending the last chunk, and the issue remain. mod_proxy_http
addresses retries for requests *with body* by the "ping" parameter, maybe
it could also retry idempotent requests (like mod_proxy_ajp does), but this
is still not an overall solution, is there one?



>
> Re-thinking the proposed flush mechanism, I don't think using the flushing
> will really solve the problem, albeit it probably narrowing down the window
> of opportunity for the problem significantly in most scenarios. I mention
> this because I'm getting the feeling two different discussions are being
> mixed up: Solving the
> 
> "keep-alive time out" problem and introducing the
> 
> "flush-all-immediately" option. While the latter might improve the
> situation with the first, it is no complete solution and there are a lot of
> scenarios where it does not apply due to other factors like filters (as
> discussed previously).
>

See above, the flushall does not resolve the issue (filters...), but the
early prefetch + flushall reduces it considerably, IMHO.


>
> The flush option itself sounds like a good idea, so I see no reason why
> not to put it in. I just don't want to loose focus on the actual problem ;-)
>

I don't see either...

Please consider the new (yet) patch attached, can you still encounter some
"proxy: error reading status line from remote server" with it?
Even without "proxy-flushall" set, you shouldn't, whatever the time taken
to read the request first 16K bytes...

Regards,
Yann.


httpd-trunk-mod_proxy_http_prefetch+flushall.patch
Description: Binary data

Re: uds support

[Daniel Ruggeri]

On 10/16/2013 6:43 AM, Jim Jagielski wrote:
> One thing about "lumping" both the UDS path *and* the
> actual URL into the name field is that it really limits
> the size of both, such that, long term, I think it will
> come back and bite us. Since last night I've been working
> on a plan to simply create a new field for the path,
> which gives us a lot more breathing room and places
> less restrictions on URL and pathname length.

Been MIA for the past few days and I'm confused on what the final
direction is - can you clarify?

FWIW, I rather liked this idea:

On Oct 11, 2013, at 12:24 PM, Jim Jagielski  wrote:

> Committed in r1531340 the above is implemented... kinda.
> I instead went with
>
>   http://localhost/foo/bar|sock:/var/run.s.sock
>
> which worked out just a bit cleaner...

... because as a server administrator, it's fairly clear what's going on.

Can you elaborate on what the challenges were with merging the two
paths? Why merge them at all? It seems logical that everything after the
pipe is used to open the socket and everything prior is treated as it
has always been and never shall the two mix... or am I over simplifying
things?

IMO, the syntax you suggested on the 11th also allows for a bit of
"futureproofing" in that "sock:" can be replaced with all kinds of
things down the road. Maybe some day I'll have an ethernet cord plugged
into my ear and it'll become
http://localhost/memory|brain:/pub/wetware.sock :-)

Also
Per some other discussion, it seems like using localhost as HTTP host is
too restrictive. I'd hate to think that a UDS backend can't implement
its own concept of name-based vhosts behind Apache because localhost is
forced as the Host header.

--
Daniel Ruggeri

RE: error log providers, multiple vhosts, mod_syslog

[Plüm , Rüdiger , Vodafone Group]

> -Original Message-
> From: Jan Kaluža 
> Sent: Donnerstag, 17. Oktober 2013 13:21
> To: dev@httpd.apache.org
> Subject: Re: error log providers, multiple vhosts, mod_syslog
> 
> There's another problem with log providers and vhosts and I think I have
> no idea how to fix it without doing dirty hacks...
> 
> The problem is with ap_open_logs function, which does following:
> 
> 1. Main server log is opened (open_error_log()). If this log uses error
> log provider, s_main->error_log is set to NULL.
> 
> 2. When there is no s_main->error_log, stderr is redirected to /dev/null.

Hmm. This points out another issue when using an error log provider for the 
main server log:
We lose everything that the server or other programs like CGI-scripts write to 
the stderr FD as it
is simply written to /dev/null. Don't we need to have a separate process in 
this case that
like a piped logger reads from the reading end of the "stderr pipe" and writes 
it
via ap_server_conf->errorlog_provider->writer to the log?

> 
> 3. Error logs for vhosts are opened (another open_error_log()). If there
> is some problem when opening these logs, any ap_log_error() call is sent
> to /dev/null.

Wouldn't it try to call s->errorlog_provider->writer in line 1196 and Segfault 
if s->errorlog_provider
is NULL because of some bad "classic" configuration that simply failed?

Regards

Rüdiger

Re: error log providers, multiple vhosts, mod_syslog

[Jan Kaluža]

There's another problem with log providers and vhosts and I think I have 
no idea how to fix it without doing dirty hacks...


The problem is with ap_open_logs function, which does following:

1. Main server log is opened (open_error_log()). If this log uses error 
log provider, s_main->error_log is set to NULL.


2. When there is no s_main->error_log, stderr is redirected to /dev/null.

3. Error logs for vhosts are opened (another open_error_log()). If there 
is some problem when opening these logs, any ap_log_error() call is sent 
to /dev/null.


The same happens for any further error message logged without server_rec *.

I think that what we need is to log to stderr_log only when it points to 
valid location (real stderr or some real file), but when it's set to 
/dev/null, we should use main server's error log provider.


I have created patch (attached) which does that, but I'm not satisfied 
with the way how it works... Maybe someone has better idea how to fix 
this issue?


Thanks,
Jan Kaluza

On 10/15/2013 05:27 PM, Jeff Trawick wrote:

Does this patch/commit look okay?  It works for me with a simple
provider in different scenarios (vhost that inherits provider setup from
s_main, vhost that has its own setup).

I suppose mod_syslog needs to disallow any attempts to configure it in a
vhost with different settings since openlog() is process-wide.  Jan, can
you look at that by chance?

-- Forwarded message --
From: ** mailto:traw...@apache.org>>
Date: Tue, Oct 15, 2013 at 10:09 AM
Subject: svn commit: r1532344 - /httpd/httpd/trunk/server/log.c
To: c...@httpd.apache.org 


Author: trawick
Date: Tue Oct 15 14:09:29 2013
New Revision: 1532344

URL: http://svn.apache.org/r1532344
Log:
Follow-up to r1525597:

Initialize error log providers in vhosts, solving crashes
when logging from those vhosts as well as allowing a different
provider (or provider configuration) for vhosts.

Modified:
 httpd/httpd/trunk/server/log.c

Modified: httpd/httpd/trunk/server/log.c
URL:
http://svn.apache.org/viewvc/httpd/httpd/trunk/server/log.c?rev=1532344&r1=1532343&r2=1532344&view=diff
==
--- httpd/httpd/trunk/server/log.c (original)
+++ httpd/httpd/trunk/server/log.c Tue Oct 15 14:09:29 2013
@@ -458,6 +458,18 @@ int ap_open_logs(apr_pool_t *pconf, apr_
  virt->error_log = q->error_log;
  }
  }
+else if (virt->errorlog_provider) {
+/* separately-configured vhost-specific provider */
+if (open_error_log(virt, 0, p) != OK) {
+return DONE;
+}
+}
+else if (s_main->errorlog_provider) {
+/* inherit provider from s_main */
+virt->errorlog_provider = s_main->errorlog_provider;
+virt->errorlog_provider_handle =
s_main->errorlog_provider_handle;
+virt->error_log = NULL;
+}
  else {
  virt->error_log = s_main->error_log;
  }





--
Born in Roswell... married an alien...
http://emptyhammock.com/


Index: server/log.c
===
--- server/log.c	(revision 1532978)
+++ server/log.c	(working copy)
@@ -435,9 +435,12 @@
 #define NULL_DEVICE "/dev/null"
 #endif
 
-if (replace_stderr && freopen(NULL_DEVICE, "w", stderr) == NULL) {
-ap_log_error(APLOG_MARK, APLOG_CRIT, errno, s_main, APLOGNO(00093)
- "unable to replace stderr with %s", NULL_DEVICE);
+if (replace_stderr) {
+if (freopen(NULL_DEVICE, "w", stderr) == NULL) {
+ap_log_error(APLOG_MARK, APLOG_CRIT, errno, s_main, APLOGNO(00093)
+"unable to replace stderr with %s", NULL_DEVICE);
+}
+stderr_log = NULL;
 }
 
 for (virt = s_main->next; virt; virt = virt->next) {
@@ -1032,6 +1035,8 @@
 const request_rec *rmain = NULL;
 core_server_config *sconf = NULL;
 ap_errorlog_info info;
+ap_errorlog_provider *errorlog_provider = NULL;
+void *errorlog_provider_handle = NULL;
 
 /* do we need to log once-per-req or once-per-conn info? */
 int log_conn_info = 0, log_req_info = 0;
@@ -1058,6 +1063,10 @@
 #endif
 
 logf = stderr_log;
+if (!logf && ap_server_conf && ap_server_conf->errorlog_provider) {
+errorlog_provider = ap_server_conf->errorlog_provider;
+errorlog_provider_handle = ap_server_conf->errorlog_provider_handle;
+}
 }
 else {
 int configured_level = r ? ap_get_request_module_loglevel(r, module_index):
@@ -1076,6 +1085,9 @@
 logf = s->error_log;
 }
 
+errorlog_provider = s->errorlog_provider;
+errorlog_provider_handle = s->errorlog_provider_handle;
+
 /* the faked server_rec from mod_cgid does not have s->module_config */
 if (s->module_config) {
 sconf = ap_ge

Re: mod_proxy, oooled backend connections and the keep-alive race condition

[Thomas Eckert]

Sorry for the delayed reply. At the moment I don't have time to look at the
patch proposal in detail, sorry about that too. I'll get back to it soon, I
hope.

> Pre-fetching 16K (or waiting for input filters to provide these) is not
always a fast operation, and the case where the backend closes its
connection in the meantime is easy to
> reproduce (and even likely to happen with some applications).

I absolutely agree. That's why I proposed to solve this more systematically
then trial-and-error-like in the first place.


> Hence, why cannot mod_proxy_http prefetch the client's body *before*
trying anything with the backend connection, and only if it's all good,
connect (or reuse a connection to) the
> backend and then start forwarding the data immediatly (flushing the
prefetched ones) ?

Now I'm confused. Are we talking about flushing everything immediately from
client to backend (that's what I understood from your first patch proposal
in this thread) or are we talking about pre fetching the whole request body
from the client and then passing it on to the backend as a whole ?


The problem I mentioned in the first message is about treating the backend
connection in an error prone way. By not ensuring the connection is still
valid - e.g. as seen by the peer - we risk running into this problem no
matter what we do elsewhere. So I really think the proper way to handle
this is to reopen the connection if necessary - be this with a filter or
integrated into the connection handling itself - and only fail after we
tried at least once.

Re-thinking the proposed flush mechanism, I don't think using the flushing
will really solve the problem, albeit it probably narrowing down the window
of opportunity for the problem significantly in most scenarios. I mention
this because I'm getting the feeling two different discussions are being
mixed up: Solving the "keep-alive time out" problem and introducing the
"flush-all-immediately" option. While the latter might improve the
situation with the first, it is no complete solution and there are a lot of
scenarios where it does not apply due to other factors like filters (as
discussed previously).

The flush option itself sounds like a good idea, so I see no reason why not
to put it in. I just don't want to loose focus on the actual problem ;-)

Cheers,
  Thomas







On Mon, Oct 7, 2013 at 6:49 PM, Yann Ylavic  wrote:

> Sorry to insist about this issue but I don't see how the current
> mod_proxy_http's behaviour of connecting the backend (or checking
> established connection reusability) before prefetching the client's body is
> not a problem.
>
> Pre-fetching 16K (or waiting for input filters to provide these) is not
> always a fast operation, and the case where the backend closes its
> connection in the meantime is easy to reproduce (and even likely to happen
> with some applications).
>
> Hence, why cannot mod_proxy_http prefetch the client's body *before*
> trying anything with the backend connection, and only if it's all good,
> connect (or reuse a connection to) the backend and then start forwarding
> the data immediatly (flushing the prefetched ones) ?
>
> By doing this, the time between the connect (or check) and the first bytes
> sent to the backend is minimal.
>
> It does not require to disable the prefetch since this one can really help
> the decision about forwarding Content-Length vs chunked (which may not be
> handled by the backend, IFAIK this is not a HTTP/1.1 requirement to handle
> it for requests).
>
> That was the purpose of the first patch I proposed ealier, but had no
> feedback...
> Maybe this message and the following patch could have more chance.
>
> This patch is quite the same as the previous one (ap_proxy_http_request
> split in 2, ap_proxy_http_prefetch to prefetch before connect and
> ap_proxy_http_request to forward using the relevent
> stream_reqbody_cl/chunked once prefetched and connected). It just fixes the
> immediate flush of the prefetched bytes when it's time to forward, and the
> handling of backend->close set by ap_proxy_create_hdrbrgd (or
> ap_proxy_http_prefetch) before the connection even exist (or the previous
> one to be reused).
>
> Regards,
> Yann.
>
> Index: modules/proxy/mod_proxy_http.c
> ===
> --- modules/proxy/mod_proxy_http.c(revision 1529127)
> +++ modules/proxy/mod_proxy_http.c(working copy)
> @@ -251,6 +251,7 @@ static int stream_reqbody_chunked(apr_pool_t *p,
>  while (!APR_BUCKET_IS_EOS(APR_BRIGADE_FIRST(input_brigade)))
>  {
>  char chunk_hdr[20];  /* must be here due to transient bucket. */
> +int flush = 0;
>
>  /* If this brigade contains EOS, either stop or remove it. */
>  if (APR_BUCKET_IS_EOS(APR_BRIGADE_LAST(input_brigade))) {
> @@ -299,13 +300,14 @@ static int stream_reqbody_chunked(apr_pool_t *p,
>  }
>
>  header_brigade = NULL;
> +flush = 1;
>  }
>