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

[Yann Ylavic]

On Tue, Jun 28, 2016 at 12:11 AM, Yann Ylavic  wrote:
> On Mon, Jun 27, 2016 at 11:26 PM, Yann Ylavic  wrote:
>> On Mon, Jun 27, 2016 at 10:48 AM, Stefan Eissing
>>  wrote:
>>>
 Am 27.06.2016 um 10:41 schrieb Yann Ylavic :

 On Mon, Jun 27, 2016 at 10:23 AM, Stefan Eissing  
 wrote:
> This looks nice for HTTP/1.1, but what about other protocols? Do I read 
> it correctly that any pending data downstream will reopen the connection?

 Hmm, I did not think about mod_proxy_h2, but correct (I'd rather say
 upstream though, backend side).
 Are there cases where some backend data are available before the
 request is sent?
>>>
>>> In HTTP/2, yes. For example a PING frame might be waiting, or a
>>> SETTINGS change. Most backends will have short timeouts for answers
>>> to these (SETTINGS is expected to be ACKed soonish), but races may
>>> happen. Also, HTTP/2 extensions with new frame types that need to be
>>> ignored when not known may get received here.
>>
>> OK, so we probably should get rid of the call to
>> ap_proxy_ssl_connection_cleanup() in proxy_http2_handler (and also in
>> proxy_http_handler with my latest changes since the same check is now
>> available in ap_proxy_check_connection).
>
> I went ahead and committed r1750414 (resp. r1750416).
> The possible issue if r1750414 were backported, is that without
> r1750392 mod_proxy_http2 may not detect a TLS close notify before
> reusing a backend connection.
> If it's not backported, it may close a legitimate backend connection
> with (pre-)available data...

I meant: it may discard (pre-)available data (not closing the connection).

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

[Yann Ylavic]

On Mon, Jun 27, 2016 at 11:26 PM, Yann Ylavic  wrote:
> On Mon, Jun 27, 2016 at 10:48 AM, Stefan Eissing
>  wrote:
>>
>>> Am 27.06.2016 um 10:41 schrieb Yann Ylavic :
>>>
>>> On Mon, Jun 27, 2016 at 10:23 AM, Stefan Eissing  wrote:
 This looks nice for HTTP/1.1, but what about other protocols? Do I read it 
 correctly that any pending data downstream will reopen the connection?
>>>
>>> Hmm, I did not think about mod_proxy_h2, but correct (I'd rather say
>>> upstream though, backend side).
>>> Are there cases where some backend data are available before the
>>> request is sent?
>>
>> In HTTP/2, yes. For example a PING frame might be waiting, or a
>> SETTINGS change. Most backends will have short timeouts for answers
>> to these (SETTINGS is expected to be ACKed soonish), but races may
>> happen. Also, HTTP/2 extensions with new frame types that need to be
>> ignored when not known may get received here.
>
> OK, so we probably should get rid of the call to
> ap_proxy_ssl_connection_cleanup() in proxy_http2_handler (and also in
> proxy_http_handler with my latest changes since the same check is now
> available in ap_proxy_check_connection).

I went ahead and committed r1750414 (resp. r1750416).
The possible issue if r1750414 were backported, is that without
r1750392 mod_proxy_http2 may not detect a TLS close notify before
reusing a backend connection.
If it's not backported, it may close a legitimate backend connection
with (pre-)available data...

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

[Yann Ylavic]

On Mon, Jun 27, 2016 at 10:48 AM, Stefan Eissing
 wrote:
>
>> Am 27.06.2016 um 10:41 schrieb Yann Ylavic :
>>
>> On Mon, Jun 27, 2016 at 10:23 AM, Stefan Eissing  wrote:
>>> This looks nice for HTTP/1.1, but what about other protocols? Do I read it 
>>> correctly that any pending data downstream will reopen the connection?
>>
>> Hmm, I did not think about mod_proxy_h2, but correct (I'd rather say
>> upstream though, backend side).
>> Are there cases where some backend data are available before the
>> request is sent?
>
> In HTTP/2, yes. For example a PING frame might be waiting, or a
> SETTINGS change. Most backends will have short timeouts for answers
> to these (SETTINGS is expected to be ACKed soonish), but races may
> happen. Also, HTTP/2 extensions with new frame types that need to be
> ignored when not known may get received here.

OK, so we probably should get rid of the call to
ap_proxy_ssl_connection_cleanup() in proxy_http2_handler (and also in
proxy_http_handler with my latest changes since the same check is now
available in ap_proxy_check_connection).

Regards,
Yann.

Re: svn commit: r1750392 - in /httpd/httpd/trunk: CHANGES docs/log-message-tags/next-number include/ap_mmn.h modules/proxy/mod_proxy.h modules/proxy/mod_proxy_ajp.c modules/proxy/mod_proxy_fcgi.c modu

[Yann Ylavic]

On Mon, Jun 27, 2016 at 10:25 PM, Yann Ylavic  wrote:
> I thought we could later follow up on this change and optimize these
> by using the new tmp_bb field (cleanup is faster than
> create/destroy)...

Something like the attached patch.
Index: modules/proxy/mod_proxy_http.c
===
--- modules/proxy/mod_proxy_http.c  (revision 1750392)
+++ modules/proxy/mod_proxy_http.c  (working copy)
@@ -1253,7 +1253,7 @@ int ap_proxy_http_process_response(apr_pool_t * p,
 const char *buf;
 char keepchar;
 apr_bucket *e;
-apr_bucket_brigade *bb, *tmp_bb;
+apr_bucket_brigade *bb;
 apr_bucket_brigade *pass_bb;
 int len, backasswards;
 int interim_response = 0; /* non-zero whilst interim 1xx responses
@@ -1306,16 +1306,17 @@ int ap_proxy_http_process_response(apr_pool_t * p,
 backend->r->proxyreq = PROXYREQ_RESPONSE;
 apr_table_setn(r->notes, "proxy-source-port", apr_psprintf(r->pool, "%hu",
origin->local_addr->port));
-tmp_bb = apr_brigade_create(p, c->bucket_alloc);
 do {
 apr_status_t rc;
 
 apr_brigade_cleanup(bb);
 
-rc = ap_proxygetline(tmp_bb, buffer, sizeof(buffer), backend->r, 0, 
);
+rc = ap_proxygetline(backend->tmp_bb, buffer, sizeof(buffer),
+ backend->r, 0, );
 if (len == 0) {
 /* handle one potential stray CRLF */
-rc = ap_proxygetline(tmp_bb, buffer, sizeof(buffer), backend->r, 
0, );
+rc = ap_proxygetline(backend->tmp_bb, buffer, sizeof(buffer),
+ backend->r, 0, );
 }
 if (len <= 0) {
 ap_log_rerror(APLOG_MARK, APLOG_ERR, rc, r, APLOGNO(01102)
Index: modules/proxy/proxy_util.c
===
--- modules/proxy/proxy_util.c  (revision 1750392)
+++ modules/proxy/proxy_util.c  (working copy)
@@ -1298,6 +1298,7 @@ PROXY_DECLARE(apr_status_t) ap_proxy_initialize_ba
 static void socket_cleanup(proxy_conn_rec *conn)
 {
 conn->sock = NULL;
+conn->tmp_bb = NULL;
 conn->connection = NULL;
 conn->ssl_hostname = NULL;
 apr_pool_clear(conn->scpool);
@@ -1401,7 +1402,6 @@ static apr_status_t connection_cleanup(void *theco
 PROXY_DECLARE(apr_status_t) ap_proxy_ssl_connection_cleanup(proxy_conn_rec 
*conn,
 request_rec *r)
 {
-apr_bucket_brigade *bb;
 apr_status_t rv;
 
 /*
@@ -1413,22 +1413,21 @@ PROXY_DECLARE(apr_status_t) ap_proxy_ssl_connectio
  * processed. We don't expect any data to be in the returned brigade.
  */
 if (conn->sock && conn->connection) {
-bb = apr_brigade_create(r->pool, r->connection->bucket_alloc);
-rv = ap_get_brigade(conn->connection->input_filters, bb,
+rv = ap_get_brigade(conn->connection->input_filters, conn->tmp_bb,
 AP_MODE_READBYTES, APR_NONBLOCK_READ,
 HUGE_STRING_LEN);
 if ((rv != APR_SUCCESS) && !APR_STATUS_IS_EAGAIN(rv)) {
 socket_cleanup(conn);
 }
-if (!APR_BRIGADE_EMPTY(bb)) {
+if (!APR_BRIGADE_EMPTY(conn->tmp_bb)) {
 apr_off_t len;
 
-rv = apr_brigade_length(bb, 0, );
+rv = apr_brigade_length(conn->tmp_bb, 0, );
 ap_log_rerror(APLOG_MARK, APLOG_TRACE3, rv, r,
   "SSL cleanup brigade contained %"
   APR_OFF_T_FMT " bytes of data.", len);
+apr_brigade_cleanup(conn->tmp_bb);
 }
-apr_brigade_destroy(bb);
 }
 return APR_SUCCESS;
 }
@@ -2712,9 +2711,6 @@ PROXY_DECLARE(apr_status_t) ap_proxy_check_backend
 
 if (conn->connection) {
 conn_rec *c = conn->connection;
-if (conn->tmp_bb == NULL) {
-conn->tmp_bb = apr_brigade_create(c->pool, c->bucket_alloc);
-}
 rv = ap_get_brigade(c->input_filters, conn->tmp_bb,
 AP_MODE_SPECULATIVE, APR_NONBLOCK_READ, 1);
 if (rv == APR_SUCCESS && expect_empty) {
@@ -3046,6 +3042,7 @@ static int proxy_connection_create(const char *pro
 }
 
 bucket_alloc = apr_bucket_alloc_create(conn->scpool);
+conn->tmp_bb = apr_brigade_create(conn->scpool, bucket_alloc);
 /*
  * The socket is now open, create a new backend server connection
  */

Re: svn commit: r1750335 - /httpd/httpd/trunk/acinclude.m4

[William A Rowe Jr]

On Mon, Jun 27, 2016 at 10:14 AM, William A Rowe Jr 
wrote:

>
> On Jun 27, 2016 9:44 AM, "Ruediger Pluem"  wrote:
> >
> >
> >
> > On 06/27/2016 04:35 PM, William A Rowe Jr wrote:
> > > On Mon, Jun 27, 2016 at 9:25 AM, Ruediger Pluem  > wrote:
> > >
> > > > --- httpd/httpd/trunk/acinclude.m4 (original)
> > > > +++ httpd/httpd/trunk/acinclude.m4 Mon Jun 27 13:45:02 2016
> > > > @@ -375,6 +375,8 @@ AC_DEFUN([APACHE_MODULE],[
> > > >  "$force_$1" != "no" ; then
> > > >enable_$1=$module_default
> > > >_apmod_extra_msg=" ($module_selection)"
> > > > +  else
> > > > + enable_$1=no
> > >
> > > What if enable_$1 is set to shared by the the user of ./configure?
> Wouldn't that get overriden here?
> > >
> > > The valid cases of 'yes', 'shared', 'static', 'no', 'most' (??),
> 'maybe-all' (??), are all handled
> > > in the various if/elif cases above this change.
> >
> > I don't see the handling of 'shared' above, but maybe I missed it. Hence
> my question. I see static, yes, maybe-all,
> > most, but not shared.
>
> My eyes playing tricks on me, you are right.
>
> Running some various tests, hope to complete the fix this afternoon once I
> come up with any other obscure edge cases.
>
> > > The edge case of 'few' and 'reallyall' are not handled above, and we
> perhaps need to account
> > > for these? In any case, the old logic seems to always result in
> build-shared for unexpected
> > > values, no matter how silly they are (e.g. 'none').
> >
> > Agreed that it should be fixed.
>
Hopefully with the last commit it is... now exhaustively testing for the
rest of today

and looking for other strange cases.

> I still don't understand what --enable-modules=few (etc) are even supposed
> to mean.
>
... but am trying to unwind where that can come into play for enable_foo
(for mod_foo)

Re: svn commit: r1750392 - in /httpd/httpd/trunk: CHANGES docs/log-message-tags/next-number include/ap_mmn.h modules/proxy/mod_proxy.h modules/proxy/mod_proxy_ajp.c modules/proxy/mod_proxy_fcgi.c modu

[Jim Jagielski]

> On Jun 27, 2016, at 1:26 PM, yla...@apache.org wrote:
> 
> +apr_bucket_brigade *tmp_bb;
> } proxy_conn_rec;
> 

I am missing the reason why this brigade needs to be
a field in this struct. Is it simply to prevent us having
to create it during each call of ap_proxy_check_backend()?

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

[Yann Ylavic]

On Mon, Jun 27, 2016 at 6:00 PM, Yann Ylavic  wrote:
>
> PR 57832 does not depend on this (race) condition, hence this change
> (r1750301, or actually its successor since I'll revert it for a less
> intrusive version) could be applied to 2.4.x without r1656259.

New commit is r1750392.
I'll let it linger in trunk (for review) before any backport proposal...

Re: T 2.4.23 tomorrow (Thurs) ??

[Jens Schleusener]

On Mon, 27 Jun 2016, William A Rowe Jr wrote:


On Mon, Jun 27, 2016 at 6:00 AM, Jens Schleusener 
 wrote:
  On Fri, 24 Jun 2016, William A Rowe Jr wrote:

On Thu, Jun 23, 2016 at 10:38 AM, Jens Schleusener 

wrote:

1) Just a pure ./configure (probably equivalent to using the option 
--enable-mods-shared=most) let
produce the "make" (compared to the last release 2.4.20) 86 modules instead 85 
modules with the
additional modules

 modules/core/.libs/mod_watchdog.so
 modules/proxy/.libs/mod_proxy_hcheck.so


Yes, proposed (and accepted) that we change the watchdog default to enable
at enable "most", as proxy_hcheck requires it.
 
  and the omitted module

   modules/session/.libs/mod_session_crypto.so

  That may be expected.


Need to look at mod_session_crypto, I hadn't proposed mod_session 
changes, modules/session/config.m4 hasn't changed since 2.4.21 was 
tagged.  acinclude.m4 changes may have had side effects, but unlikely.
Once the whole "--enable-proxy" decision is made, others that followed 
this broken pattern need to be reevaluated, but that isn't a change for 
a 2.4.23 tag, since there should be no regression here.

There is a simpler explanation, did you change the detected apr-util
to one without apr_crypto (based on openssl) enabled? Ensure your
apr-util build enabled openssl and is current (1.5).


Oops, my fault, well combined, you are completely right: For 2.4.20 I
used (since I have originally not downloaded httpd-2.4.20-deps.tar.bz2) 
my "separate" self-compiled apr-1.5.2 and apr-util-1.5.4 
installatations (using the configure options --with-crypto 
--with-openssl=/usr).



  By the way the option --enable-mods-shared=all produces 103 modules and
  --enable-mods-shared=reallyall 120 ones.

  2) But for me surprising the option--enable-mods-shared=none seems to 
have the same configuration
  effect as the option--enable-mods-shared=most (producing 86 modules) 
respectively the option is
  ignored and the default "most" is used.

  Similar for e.g. --enable-mods-shared='headers rewrite dav' (an example 
from the documentation
  page http://httpd.apache.org/docs/2.4/programs/configure.html) seems to 
produce the same behaviour
  like --enable-mods-shared=most while I would expect that only the three 
specified modules would be
  buillt.

  Odd, also arbitray module names like --enable-mods-shared=nonsense seems 
equivalent to
  --enable-mods-shared=most.

  But that is an error it seems not a regression since 2.4.20 shows the 
same behaviour.


It is, you found it, and the error was already a side effect within acinclude.m4
which trusted the value 'no' (which is why I continued to trust the value 'no').

In order for --enable-mods-shared=nonsense to behave as expected, I think
the patch is pretty simple...

--- acinclude.m4 (revision 1749925)
+++ acinclude.m4 (working copy)
@@ -375,6 +375,8 @@
             "$force_$1" != "no" ; then
       enable_$1=$module_default
       _apmod_extra_msg=" ($module_selection)"
+  else
+     enable_$1=no
   fi
   if test "$enable_$1" != "no"; then
     dnl If we plan to enable it, allow the module to run some autoconf magic


Just for completeness (since another fix seems on the way): At some first 
quick tests this fix seems not to work for me. For httpd-2.4.x I saw no 
difference in the number of built modules using either 
--enable-mods-shared=nonsense or --enable-mods-shared=none and for 
httpd-2.4.21 (patched with your proxy-rollup-2.4.21.patch patch) it let 
only omit the modules


 modules/proxy/balancers/.libs/mod_lbmethod_bybusyness.so
 modules/proxy/balancers/.libs/mod_lbmethod_byrequests.so
 modules/proxy/balancers/.libs/mod_lbmethod_bytraffic.so
 modules/proxy/balancers/.libs/mod_lbmethod_heartbeat.so

Regards

Jens

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

[Yann Ylavic]

On Mon, Jun 27, 2016 at 3:17 PM, Jim Jagielski  wrote:
> Doesn't this depend on:
>
>  trunk patch: http://svn.apache.org/r1656259
>   http://svn.apache.org/r1656359 (CHANGES entry)
>
> which, in STATUS, is tagged as still being worked?

I don't think so, r1656259 is more about reducing the time frame
between the backend connection check and its effective reuse (for
requests with a body which may be long to read or retained by some
input filter).

PR 57832 does not depend on this (race) condition, hence this change
(r1750301, or actually its successor since I'll revert it for a less
intrusive version) could be applied to 2.4.x without r1656259.

No hurry though, this is mainly to mitigate a (potential) defect on
the backend side only, it could be included in > 2.4.23 IMHO.

Regards,
Yann.

Re: svn commit: r1750335 - /httpd/httpd/trunk/acinclude.m4

[William A Rowe Jr]

On Jun 27, 2016 9:44 AM, "Ruediger Pluem"  wrote:
>
>
>
> On 06/27/2016 04:35 PM, William A Rowe Jr wrote:
> > On Mon, Jun 27, 2016 at 9:25 AM, Ruediger Pluem > wrote:
> >
> >
> > On 06/27/2016 03:45 PM, wr...@apache.org 
wrote:
> > > Author: wrowe
> > > Date: Mon Jun 27 13:45:02 2016
> > > New Revision: 1750335
> > >
> > > URL: http://svn.apache.org/viewvc?rev=1750335=rev
> > > Log:
> > > Ensure not-selected means 'no', once an APACHE_MODULE enable_foo
is processed
> > >
> > > Modified:
> > > httpd/httpd/trunk/acinclude.m4
> > >
> > > Modified: httpd/httpd/trunk/acinclude.m4
> > > URL:
http://svn.apache.org/viewvc/httpd/httpd/trunk/acinclude.m4?rev=1750335=1750334=1750335=diff
> > >
==
> > > --- httpd/httpd/trunk/acinclude.m4 (original)
> > > +++ httpd/httpd/trunk/acinclude.m4 Mon Jun 27 13:45:02 2016
> > > @@ -375,6 +375,8 @@ AC_DEFUN([APACHE_MODULE],[
> > >  "$force_$1" != "no" ; then
> > >enable_$1=$module_default
> > >_apmod_extra_msg=" ($module_selection)"
> > > +  else
> > > + enable_$1=no
> >
> > What if enable_$1 is set to shared by the the user of ./configure?
Wouldn't that get overriden here?
> >
> >
> > The valid cases of 'yes', 'shared', 'static', 'no', 'most' (??),
'maybe-all' (??), are all handled
> > in the various if/elif cases above this change.
>
> I don't see the handling of 'shared' above, but maybe I missed it. Hence
my question. I see static, yes, maybe-all,
> most, but not shared.

My eyes playing tricks on me, you are right.

Running some various tests, hope to complete the fix this afternoon once I
come up with any other obscure edge cases.

> > The edge case of 'few' and 'reallyall' are not handled above, and we
perhaps need to account
> > for these? In any case, the old logic seems to always result in
build-shared for unexpected
> > values, no matter how silly they are (e.g. 'none').
> >
>
> Agreed that it should be fixed.

I still don't understand what --enable-modules=few (etc) are even supposed
to mean.

Re: svn commit: r1750335 - /httpd/httpd/trunk/acinclude.m4

[Ruediger Pluem]

On 06/27/2016 04:35 PM, William A Rowe Jr wrote:
> On Mon, Jun 27, 2016 at 9:25 AM, Ruediger Pluem  > wrote:
> 
> 
> On 06/27/2016 03:45 PM, wr...@apache.org  wrote:
> > Author: wrowe
> > Date: Mon Jun 27 13:45:02 2016
> > New Revision: 1750335
> >
> > URL: http://svn.apache.org/viewvc?rev=1750335=rev
> > Log:
> > Ensure not-selected means 'no', once an APACHE_MODULE enable_foo is 
> processed
> >
> > Modified:
> > httpd/httpd/trunk/acinclude.m4
> >
> > Modified: httpd/httpd/trunk/acinclude.m4
> > URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/acinclude.m4?rev=1750335=1750334=1750335=diff
> > 
> ==
> > --- httpd/httpd/trunk/acinclude.m4 (original)
> > +++ httpd/httpd/trunk/acinclude.m4 Mon Jun 27 13:45:02 2016
> > @@ -375,6 +375,8 @@ AC_DEFUN([APACHE_MODULE],[
> >  "$force_$1" != "no" ; then
> >enable_$1=$module_default
> >_apmod_extra_msg=" ($module_selection)"
> > +  else
> > + enable_$1=no
> 
> What if enable_$1 is set to shared by the the user of ./configure? 
> Wouldn't that get overriden here?
> 
> 
> The valid cases of 'yes', 'shared', 'static', 'no', 'most' (??), 'maybe-all' 
> (??), are all handled 
> in the various if/elif cases above this change.

I don't see the handling of 'shared' above, but maybe I missed it. Hence my 
question. I see static, yes, maybe-all,
most, but not shared.

> 
> The edge case of 'few' and 'reallyall' are not handled above, and we perhaps 
> need to account
> for these? In any case, the old logic seems to always result in build-shared 
> for unexpected
> values, no matter how silly they are (e.g. 'none').
> 

Agreed that it should be fixed.

Regards

Rüdiger

Re: svn commit: r1750335 - /httpd/httpd/trunk/acinclude.m4

[William A Rowe Jr]

On Mon, Jun 27, 2016 at 9:25 AM, Ruediger Pluem  wrote:

>
> On 06/27/2016 03:45 PM, wr...@apache.org wrote:
> > Author: wrowe
> > Date: Mon Jun 27 13:45:02 2016
> > New Revision: 1750335
> >
> > URL: http://svn.apache.org/viewvc?rev=1750335=rev
> > Log:
> > Ensure not-selected means 'no', once an APACHE_MODULE enable_foo is
> processed
> >
> > Modified:
> > httpd/httpd/trunk/acinclude.m4
> >
> > Modified: httpd/httpd/trunk/acinclude.m4
> > URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/acinclude.m4?rev=1750335=1750334=1750335=diff
> >
> ==
> > --- httpd/httpd/trunk/acinclude.m4 (original)
> > +++ httpd/httpd/trunk/acinclude.m4 Mon Jun 27 13:45:02 2016
> > @@ -375,6 +375,8 @@ AC_DEFUN([APACHE_MODULE],[
> >  "$force_$1" != "no" ; then
> >enable_$1=$module_default
> >_apmod_extra_msg=" ($module_selection)"
> > +  else
> > + enable_$1=no
>
> What if enable_$1 is set to shared by the the user of ./configure?
> Wouldn't that get overriden here?
>

The valid cases of 'yes', 'shared', 'static', 'no', 'most' (??),
'maybe-all' (??), are all handled
in the various if/elif cases above this change.

The edge case of 'few' and 'reallyall' are not handled above, and we
perhaps need to account
for these? In any case, the old logic seems to always result in
build-shared for unexpected
values, no matter how silly they are (e.g. 'none').

Re: svn commit: r1750335 - /httpd/httpd/trunk/acinclude.m4

[Ruediger Pluem]

On 06/27/2016 03:45 PM, wr...@apache.org wrote:
> Author: wrowe
> Date: Mon Jun 27 13:45:02 2016
> New Revision: 1750335
> 
> URL: http://svn.apache.org/viewvc?rev=1750335=rev
> Log:
> Ensure not-selected means 'no', once an APACHE_MODULE enable_foo is processed
> 
> Modified:
> httpd/httpd/trunk/acinclude.m4
> 
> Modified: httpd/httpd/trunk/acinclude.m4
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/acinclude.m4?rev=1750335=1750334=1750335=diff
> ==
> --- httpd/httpd/trunk/acinclude.m4 (original)
> +++ httpd/httpd/trunk/acinclude.m4 Mon Jun 27 13:45:02 2016
> @@ -375,6 +375,8 @@ AC_DEFUN([APACHE_MODULE],[
>  "$force_$1" != "no" ; then
>enable_$1=$module_default
>_apmod_extra_msg=" ($module_selection)"
> +  else
> + enable_$1=no

What if enable_$1 is set to shared by the the user of ./configure? Wouldn't 
that get overriden here?

Regards

Rüdiger

Re: T 2.4.23 tomorrow (Thurs) ??

[William A Rowe Jr]

On Mon, Jun 27, 2016 at 6:00 AM, Jens Schleusener <
jens.schleuse...@t-online.de> wrote:

> On Fri, 24 Jun 2016, William A Rowe Jr wrote:
>
> On Thu, Jun 23, 2016 at 10:38 AM, Jens Schleusener <
>> jens.schleuse...@t-online.de> wrote:
>>
>> 1) Just a pure ./configure (probably equivalent to using the option
> --enable-mods-shared=most) let produce the "make" (compared to the last
> release 2.4.20) 86 modules instead 85 modules with the additional modules
>
>  modules/core/.libs/mod_watchdog.so
>  modules/proxy/.libs/mod_proxy_hcheck.so
>

Yes, proposed (and accepted) that we change the watchdog default to enable
at enable "most", as proxy_hcheck requires it.


> and the omitted module
>
>  modules/session/.libs/mod_session_crypto.so
>
> That may be expected.
>

Need to look at mod_session_crypto, I hadn't proposed mod_session
changes, modules/session/config.m4 hasn't changed since 2.4.21 was
tagged.  acinclude.m4 changes may have had side effects, but unlikely.
Once the whole "--enable-proxy" decision is made, others that followed
this broken pattern need to be reevaluated, but that isn't a change for
a 2.4.23 tag, since there should be no regression here.

There is a simpler explanation, did you change the detected apr-util
to one without apr_crypto (based on openssl) enabled? Ensure your
apr-util build enabled openssl and is current (1.5).

By the way the option --enable-mods-shared=all produces 103 modules and
> --enable-mods-shared=reallyall 120 ones.
>
> 2) But for me surprising the option--enable-mods-shared=none seems to have
> the same configuration effect as the option--enable-mods-shared=most
> (producing 86 modules) respectively the option is ignored and the default
> "most" is used.
>
> Similar for e.g. --enable-mods-shared='headers rewrite dav' (an example
> from the documentation page
> http://httpd.apache.org/docs/2.4/programs/configure.html) seems to
> produce the same behaviour like --enable-mods-shared=most while I would
> expect that only the three specified modules would be buillt.
>
> Odd, also arbitray module names like --enable-mods-shared=nonsense seems
> equivalent to --enable-mods-shared=most.
>
> But that is an error it seems not a regression since 2.4.20 shows the same
> behaviour.
>

It is, you found it, and the error was already a side effect within
acinclude.m4
which trusted the value 'no' (which is why I continued to trust the value
'no').

In order for --enable-mods-shared=nonsense to behave as expected, I think
the patch is pretty simple...

--- acinclude.m4 (revision 1749925)
+++ acinclude.m4 (working copy)
@@ -375,6 +375,8 @@
 "$force_$1" != "no" ; then
   enable_$1=$module_default
   _apmod_extra_msg=" ($module_selection)"
+  else
+ enable_$1=no
   fi
   if test "$enable_$1" != "no"; then
 dnl If we plan to enable it, allow the module to run some autoconf
magic

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

[Jim Jagielski]

Doesn't this depend on:

 trunk patch: http://svn.apache.org/r1656259
  http://svn.apache.org/r1656359 (CHANGES entry)

which, in STATUS, is tagged as still being worked?


> On Jun 27, 2016, at 4:23 AM, Stefan Eissing  wrote:
> 
> This looks nice for HTTP/1.1, but what about other protocols? Do I read it 
> correctly that any pending data downstream will reopen the connection?
> 
>> Am 27.06.2016 um 10:00 schrieb yla...@apache.org:
>> 
>> Author: ylavic
>> Date: Mon Jun 27 08:00:30 2016
>> New Revision: 1750301
>> 
>> URL: http://svn.apache.org/viewvc?rev=1750301=rev
>> Log:
>> mod_proxy: don't reuse backend connections with data available before the
>> request is sent.  PR 57832.
>> 
>> Modified:
>>   httpd/httpd/trunk/modules/proxy/mod_proxy.h
>>   httpd/httpd/trunk/modules/proxy/proxy_util.c
>> 
>> Modified: httpd/httpd/trunk/modules/proxy/mod_proxy.h
>> URL: 
>> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy.h?rev=1750301=1750300=1750301=diff
>> ==
>> --- httpd/httpd/trunk/modules/proxy/mod_proxy.h (original)
>> +++ httpd/httpd/trunk/modules/proxy/mod_proxy.h Mon Jun 27 08:00:30 2016
>> @@ -271,6 +271,7 @@ typedef struct {
>>unsigned int inreslist:1;  /* connection in apr_reslist? */
>>const char   *uds_path;/* Unix domain socket path */
>>const char   *ssl_hostname;/* Hostname (SNI) in use by SSL connection */
>> +apr_bucket_brigade *tmp_bb;
>> } proxy_conn_rec;
>> 
>> typedef struct {
>> 
>> Modified: httpd/httpd/trunk/modules/proxy/proxy_util.c
>> URL: 
>> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/proxy_util.c?rev=1750301=1750300=1750301=diff
>> ==
>> --- httpd/httpd/trunk/modules/proxy/proxy_util.c (original)
>> +++ httpd/httpd/trunk/modules/proxy/proxy_util.c Mon Jun 27 08:00:30 2016
>> @@ -2487,7 +2487,7 @@ ap_proxy_determine_connection(apr_pool_t
>> #endif
>> 
>> #if USE_ALTERNATE_IS_CONNECTED && defined(APR_MSG_PEEK)
>> -PROXY_DECLARE(int) ap_proxy_is_socket_connected(apr_socket_t *socket)
>> +static int get_socket_connected(apr_socket_t *socket)
>> {
>>apr_pollfd_t pfds[1];
>>apr_status_t status;
>> @@ -2514,7 +2514,7 @@ PROXY_DECLARE(int) ap_proxy_is_socket_co
>>status = apr_socket_recvfrom(, socket, APR_MSG_PEEK,
>> [0], );
>>if (status == APR_SUCCESS && len)
>> -return 1;
>> +return 2;
>>else
>>return 0;
>>}
>> @@ -2525,7 +2525,7 @@ PROXY_DECLARE(int) ap_proxy_is_socket_co
>> 
>> }
>> #else
>> -PROXY_DECLARE(int) ap_proxy_is_socket_connected(apr_socket_t *socket)
>> +static int is_socket_connnected(apr_socket_t *socket)
>> 
>> {
>>apr_size_t buffer_len = 1;
>> @@ -2544,12 +2544,19 @@ PROXY_DECLARE(int) ap_proxy_is_socket_co
>>|| APR_STATUS_IS_ECONNRESET(socket_status)) {
>>return 0;
>>}
>> +else if (status == APR_SUCCESS && buffer_len) {
>> +return 2;
>> +}
>>else {
>>return 1;
>>}
>> }
>> #endif /* USE_ALTERNATE_IS_CONNECTED */
>> 
>> +PROXY_DECLARE(int) ap_proxy_is_socket_connected(apr_socket_t *socket)
>> +{
>> +return get_socket_connected(socket) != 0;
>> +}
>> 
>> /*
>> * Send a HTTP CONNECT request to a forward proxy.
>> @@ -2716,7 +2723,35 @@ PROXY_DECLARE(int) ap_proxy_connect_back
>>(proxy_server_conf *) ap_get_module_config(sconf, _module);
>> 
>>if (conn->sock) {
>> -if (!(connected = ap_proxy_is_socket_connected(conn->sock))) {
>> +conn_rec *c = conn->connection;
>> +if (!c) {
>> +connected = get_socket_connected(conn->sock);
>> +}
>> +else {
>> +if (conn->tmp_bb == NULL) {
>> +conn->tmp_bb = apr_brigade_create(c->pool, c->bucket_alloc);
>> +}
>> +rv = ap_get_brigade(c->input_filters, conn->tmp_bb,
>> +AP_MODE_SPECULATIVE, APR_NONBLOCK_READ, 1);
>> +if (rv == APR_SUCCESS) {
>> +apr_off_t len = 0;
>> +apr_brigade_length(conn->tmp_bb, 0, );
>> +if (len) {
>> +connected = 2;
>> +}
>> +else {
>> +connected = 1;
>> +}
>> +}
>> +else if (APR_STATUS_IS_EAGAIN(rv)) {
>> +connected = 1;
>> +}
>> +else {
>> +connected = 0;
>> +}
>> +apr_brigade_cleanup(conn->tmp_bb);
>> +}
>> +if (connected != 1) {
>>/* This clears conn->scpool (and associated data), so backup and
>> * restore any ssl_hostname for this connection set earlier by
>> * ap_proxy_determine_connection().
>> @@ -2728,9 +2763,17 @@ PROXY_DECLARE(int) 

Re: T 2.4.23 tomorrow (Thurs) ??

[Jens Schleusener]

On Fri, 24 Jun 2016, William A Rowe Jr wrote:


On Thu, Jun 23, 2016 at 10:38 AM, Jens Schleusener 
 wrote:

... good news.

So now nearly superfluous info but just for completeness: Your first patch for 
modules/proxy/config.m4
sent at "Thu, 23 Jun 2016 07:32:02 -0500" solved at least the superficial 
problem and let the configure
run end without an error.


The entire solution is now presented in STATUS, but for reviewers 
I've rolled up all of the diffs into a single delta that can be applied
to either 2.4.21 or 2.4.22 release candidates for easier testing;

https://raw.githubusercontent.com/wrowe/patches/master/proxy-rollup-2.4.21.patch

This will hopefully address every edge case with the proxy and
proxy sub-modules logic in ./configure (just be sure to re-run
./buildconf after applying this patch!)


Ok, I applied the patch to both release candidates and a

 ./configure --enable-mods-shared=few

ends without errors (using openSUSE Leap 42.1; autoconf-2.69-11.4.noarch; 
libtool-2.4.2-16.6.x86_64).


But a subsequent "make" produces to the expected 21 modules 
additionally the 4 modules


 modules/proxy/balancers/.libs/mod_lbmethod_bybusyness.so
 modules/proxy/balancers/.libs/mod_lbmethod_byrequests.so
 modules/proxy/balancers/.libs/mod_lbmethod_bytraffic.so
 modules/proxy/balancers/.libs/mod_lbmethod_heartbeat.so

As since yesterday httpd-2.4.x seems now accordingly patched I tried that 
branch and it also works with the above configure command and a subsequent 
"make" produces just the expected 21 modules.


So for the original reported error regarding the configure option 
--enable-mods-shared=few the fixes seems ok now.


But some observations (from an outsider):

1) Just a pure ./configure (probably equivalent to using the option 
--enable-mods-shared=most) let produce the "make" (compared to the last 
release 2.4.20) 86 modules instead 85 modules with the additional modules


 modules/core/.libs/mod_watchdog.so
 modules/proxy/.libs/mod_proxy_hcheck.so

and the omitted module

 modules/session/.libs/mod_session_crypto.so

That may be expected.

By the way the option --enable-mods-shared=all produces 103 modules and 
--enable-mods-shared=reallyall 120 ones.


2) But for me surprising the option--enable-mods-shared=none seems to have 
the same configuration effect as the option--enable-mods-shared=most 
(producing 86 modules) respectively the option is ignored and the default 
"most" is used.


Similar for e.g. --enable-mods-shared='headers rewrite dav' (an example 
from the documentation page 
http://httpd.apache.org/docs/2.4/programs/configure.html) seems to produce 
the same behaviour like --enable-mods-shared=most while I would expect 
that only the three specified modules would be buillt.


Odd, also arbitray module names like --enable-mods-shared=nonsense seems 
equivalent to --enable-mods-shared=most.


But that is an error it seems not a regression since 2.4.20 shows the same 
behaviour.


Regards

Jens

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

[Stefan Eissing]

> Am 27.06.2016 um 10:41 schrieb Yann Ylavic :
> 
> On Mon, Jun 27, 2016 at 10:23 AM, Stefan Eissing  wrote:
>> This looks nice for HTTP/1.1, but what about other protocols? Do I read it 
>> correctly that any pending data downstream will reopen the connection?
> 
> Hmm, I did not think about mod_proxy_h2, but correct (I'd rather say
> upstream though, backend side).
> Are there cases where some backend data are available before the
> request is sent?

In HTTP/2, yes. For example a PING frame might be waiting, or a SETTINGS 
change. Most backends will have short timeouts for answers to these (SETTINGS 
is expected to be ACKed soonish), but races may happen. Also, HTTP/2 extensions 
with new frame types that need to be ignored when not known may get received 
here.

Again, the added check is very good for HTTP/1, but not for protocols that 
already have request/response protection and delimiters. Can we see which proxy 
module is involved on the connection? Do we see the scheme there? That might be 
a way to whitelist the check.

-Stefan

RE: svn commit: r1750301 - in /httpd/httpd/trunk/modules/proxy: mod_proxy.h proxy_util.c

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

> -Original Message-
> From: Stefan Eissing [mailto:ste...@eissing.org]
> Sent: Montag, 27. Juni 2016 10:24
> To: dev@httpd.apache.org
> Subject: Re: svn commit: r1750301 - in /httpd/httpd/trunk/modules/proxy:
> mod_proxy.h proxy_util.c
> 
> This looks nice for HTTP/1.1, but what about other protocols? Do I read it
> correctly that any pending data downstream will reopen the connection?

Agreed. This is a severe change in behaviour and does not let other protocols 
handle this in their own way.
For them pending data might be fine.
So especially I wouldn't backport it to 2.4.x in this state.

Regards

Rüdiger

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

[Yann Ylavic]

On Mon, Jun 27, 2016 at 10:23 AM, Stefan Eissing  wrote:
> This looks nice for HTTP/1.1, but what about other protocols? Do I read it 
> correctly that any pending data downstream will reopen the connection?

Hmm, I did not think about mod_proxy_h2, but correct (I'd rather say
upstream though, backend side).
Are there cases where some backend data are available before the
request is sent?

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

[Stefan Eissing]

This looks nice for HTTP/1.1, but what about other protocols? Do I read it 
correctly that any pending data downstream will reopen the connection?

> Am 27.06.2016 um 10:00 schrieb yla...@apache.org:
> 
> Author: ylavic
> Date: Mon Jun 27 08:00:30 2016
> New Revision: 1750301
> 
> URL: http://svn.apache.org/viewvc?rev=1750301=rev
> Log:
> mod_proxy: don't reuse backend connections with data available before the
> request is sent.  PR 57832.
> 
> Modified:
>httpd/httpd/trunk/modules/proxy/mod_proxy.h
>httpd/httpd/trunk/modules/proxy/proxy_util.c
> 
> Modified: httpd/httpd/trunk/modules/proxy/mod_proxy.h
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy.h?rev=1750301=1750300=1750301=diff
> ==
> --- httpd/httpd/trunk/modules/proxy/mod_proxy.h (original)
> +++ httpd/httpd/trunk/modules/proxy/mod_proxy.h Mon Jun 27 08:00:30 2016
> @@ -271,6 +271,7 @@ typedef struct {
> unsigned int inreslist:1;  /* connection in apr_reslist? */
> const char   *uds_path;/* Unix domain socket path */
> const char   *ssl_hostname;/* Hostname (SNI) in use by SSL connection */
> +apr_bucket_brigade *tmp_bb;
> } proxy_conn_rec;
> 
> typedef struct {
> 
> Modified: httpd/httpd/trunk/modules/proxy/proxy_util.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/proxy_util.c?rev=1750301=1750300=1750301=diff
> ==
> --- httpd/httpd/trunk/modules/proxy/proxy_util.c (original)
> +++ httpd/httpd/trunk/modules/proxy/proxy_util.c Mon Jun 27 08:00:30 2016
> @@ -2487,7 +2487,7 @@ ap_proxy_determine_connection(apr_pool_t
> #endif
> 
> #if USE_ALTERNATE_IS_CONNECTED && defined(APR_MSG_PEEK)
> -PROXY_DECLARE(int) ap_proxy_is_socket_connected(apr_socket_t *socket)
> +static int get_socket_connected(apr_socket_t *socket)
> {
> apr_pollfd_t pfds[1];
> apr_status_t status;
> @@ -2514,7 +2514,7 @@ PROXY_DECLARE(int) ap_proxy_is_socket_co
> status = apr_socket_recvfrom(, socket, APR_MSG_PEEK,
>  [0], );
> if (status == APR_SUCCESS && len)
> -return 1;
> +return 2;
> else
> return 0;
> }
> @@ -2525,7 +2525,7 @@ PROXY_DECLARE(int) ap_proxy_is_socket_co
> 
> }
> #else
> -PROXY_DECLARE(int) ap_proxy_is_socket_connected(apr_socket_t *socket)
> +static int is_socket_connnected(apr_socket_t *socket)
> 
> {
> apr_size_t buffer_len = 1;
> @@ -2544,12 +2544,19 @@ PROXY_DECLARE(int) ap_proxy_is_socket_co
> || APR_STATUS_IS_ECONNRESET(socket_status)) {
> return 0;
> }
> +else if (status == APR_SUCCESS && buffer_len) {
> +return 2;
> +}
> else {
> return 1;
> }
> }
> #endif /* USE_ALTERNATE_IS_CONNECTED */
> 
> +PROXY_DECLARE(int) ap_proxy_is_socket_connected(apr_socket_t *socket)
> +{
> +return get_socket_connected(socket) != 0;
> +}
> 
> /*
>  * Send a HTTP CONNECT request to a forward proxy.
> @@ -2716,7 +2723,35 @@ PROXY_DECLARE(int) ap_proxy_connect_back
> (proxy_server_conf *) ap_get_module_config(sconf, _module);
> 
> if (conn->sock) {
> -if (!(connected = ap_proxy_is_socket_connected(conn->sock))) {
> +conn_rec *c = conn->connection;
> +if (!c) {
> +connected = get_socket_connected(conn->sock);
> +}
> +else {
> +if (conn->tmp_bb == NULL) {
> +conn->tmp_bb = apr_brigade_create(c->pool, c->bucket_alloc);
> +}
> +rv = ap_get_brigade(c->input_filters, conn->tmp_bb,
> +AP_MODE_SPECULATIVE, APR_NONBLOCK_READ, 1);
> +if (rv == APR_SUCCESS) {
> +apr_off_t len = 0;
> +apr_brigade_length(conn->tmp_bb, 0, );
> +if (len) {
> +connected = 2;
> +}
> +else {
> +connected = 1;
> +}
> +}
> +else if (APR_STATUS_IS_EAGAIN(rv)) {
> +connected = 1;
> +}
> +else {
> +connected = 0;
> +}
> +apr_brigade_cleanup(conn->tmp_bb);
> +}
> +if (connected != 1) {
> /* This clears conn->scpool (and associated data), so backup and
>  * restore any ssl_hostname for this connection set earlier by
>  * ap_proxy_determine_connection().
> @@ -2728,9 +2763,17 @@ PROXY_DECLARE(int) ap_proxy_connect_back
> }
> 
> socket_cleanup(conn);
> -ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, APLOGNO(00951)
> - "%s: backend socket is disconnected.",
> - proxy_function);
> +if (!connected) {
> +ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, APLOGNO(00951)
> +