Re: svn commit: r1836381 - in /httpd/httpd/trunk: ./ include/ modules/proxy/ modules/proxy/balancers/

[William A Rowe Jr]

On Fri, Jul 20, 2018, 15:22 Ruediger Pluem  wrote:

>
> BTW: We have the same load order issue issue with the following proxy
> modules:
>
> mod_proxy_connect
> mod_proxy_ftp
> mod_proxy_http
> mod_proxy_fcgi
> mod_proxy_scgi
> mod_proxy_fdpass
> mod_proxy_ajp
> mod_proxy_balancer
> mod_proxy_express
>

Correct. However this isn't a regression, and mod_proxy is an apparent
prerequisite to any of these, which further and thankfully sorts first. The
lbmethod providers differed in all three respects.

It might be interesting to solve in a future enhancement release, but the
number of dependencies likely makes this unrealistic.

Re: svn commit: r1836381 - in /httpd/httpd/trunk: ./ include/ modules/proxy/ modules/proxy/balancers/

[Ruediger Pluem]

On 07/20/2018 09:27 PM, rpl...@apache.org wrote:
> Author: rpluem
> Date: Fri Jul 20 19:27:31 2018
> New Revision: 1836381
> 
> URL: http://svn.apache.org/viewvc?rev=1836381=rev
> Log:
> * mod_proxy: Remove load order and link dependency between mod_lbmethod_*
>   modules and mod_proxy by providing mod_proxy's 
> ap_proxy_balancer_get_best_worker
>   as an optional function.
> 
> PR: 62557
> 

BTW: We have the same load order issue issue with the following proxy modules:

mod_proxy_connect
mod_proxy_ftp
mod_proxy_http
mod_proxy_fcgi
mod_proxy_scgi
mod_proxy_fdpass
mod_proxy_ajp
mod_proxy_balancer
mod_proxy_express

Regards

Rüdiger

Re: svn commit: r1836239 - in /httpd/httpd/trunk: include/httpd.h modules/http/http_request.c server/mpm/event/event.c server/mpm/motorz/motorz.c server/mpm/simple/simple_io.c server/util_filter.c

[Yann Ylavic]

On Fri, Jul 20, 2018 at 4:01 PM, Ruediger Pluem  wrote:
>
> You describe the inconsistency in case c->data_in_input_filters is 1,
> I did for the case that c->data_in_input_filters is 0, but yes we could have 
> inconsistencies in both cases
> and the question is if they are always fine. So far it looks like.
> But probably the flags should just go in order to avoid these cases.

Done in r1836364.

Tested/debugged with pipelining, both with event and worker, all seems
to work as expected.

Regards,
Yann.

Re: svn commit: r1832609 - in /httpd/httpd/branches/2.4.x: ./ docs/manual/howto/ docs/manual/mod/ modules/proxy/ modules/proxy/balancers/

[William A Rowe Jr]

Looks like a good direction. From PR62557, the observed modules to be
adjusted, to consume the new opt fn are;

--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -423,6 +424,9 @@ SET(mod_http2_extra_sources
   modules/http2/h2_task.cmodules/http2/h2_util.c
   modules/http2/h2_workers.c
 )
+SET(mod_lbmethod_bybusyness_extra_libs   mod_proxy)
+SET(mod_lbmethod_byrequests_extra_libs   mod_proxy)
+SET(mod_lbmethod_bytraffic_extra_libsmod_proxy)
 SET(mod_ldap_extra_defines   LDAP_DECLARE_EXPORT)
 SET(mod_ldap_extra_libs  wldap32)
 SET(mod_ldap_extra_sources


And this corresponds with


mod_lbmethod_bybusyness.c:
ap_proxy_balancer_get_best_worker(balancer, r, is_best_bybusyness,
mod_lbmethod_byrequests.c:proxy_worker *worker =
ap_proxy_balancer_get_best_worker(balancer, r, is_best_byrequests,
_factor);
mod_lbmethod_bytraffic.c:return
ap_proxy_balancer_get_best_worker(balancer, r, is_best_bytraffic,


This patch in 62557 can be ignored once the new optional entry point is
used instead, and these three modules may be loaded again prior to
mod_proxy in httpd.conf.



On Fri, Jul 20, 2018 at 5:13 AM, Ruediger Pluem  wrote:

>
>
> On 07/19/2018 08:24 PM, William A Rowe Jr wrote:
> > On Thu, May 31, 2018 at 8:24 AM, mailto:j...@apache.org>>
> wrote:
> >
> > Author: jim
> > Date: Thu May 31 13:24:04 2018
> > New Revision: 1832609
> >
> > URL: http://svn.apache.org/viewvc?rev=1832609=rev <
> http://svn.apache.org/viewvc?rev=1832609=rev>
> > Log:
> > Merge r1828890, r1832500 from trunk:
> >
> > [...]
> >
> > URL:
> > http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/
> modules/proxy/balancers/mod_lbmethod_byrequests.c?rev=
> 1832609=1832608=1832609=diff
> >  modules/proxy/balancers/mod_lbmethod_byrequests.c?rev=
> 1832609=1832608=1832609=diff>
> > 
> ==
> > --- 
> > httpd/httpd/branches/2.4.x/modules/proxy/balancers/mod_lbmethod_byrequests.c
> (original)
> > +++ 
> > httpd/httpd/branches/2.4.x/modules/proxy/balancers/mod_lbmethod_byrequests.c
> Thu May 31 13:24:04 2018
> >
> > [...]
> >
> > @@ -70,82 +77,17 @@ static int (*ap_proxy_retry_worker_fn)(c
> >   *   b a d c d a c d b d ...
> >   *
> >   */
> > -
> >  static proxy_worker *find_best_byrequests(proxy_balancer *balancer,
> >  request_rec *r)
> >  {
> > -int i;
> >  int total_factor = 0;
> >
> > [...]
> >
> > +proxy_worker *worker = ap_proxy_balancer_get_best_worker(balancer,
> r, is_best_byrequests, _factor);
> >
> >
> > This introduced a new hard runtime config ordering problem on
> mod_lbmethod_byrequest.so, which must now be loaded AFTER
> > mod_proxy.so.
> >
> > This was not previously true, as illustrated by mod_lbmethod_heartbeat,
> using the ap_proxy_retry_worker using an
> > optional function.
> >
> > lbmethod sorts before proxy, fwiw.
> >
>
> Something like the attached? The mod_lbmethod_byrequests.c part needs to
> be done for lb modules though.
>
> Regards
>
> Rüdiger
>
>
>
>

Re: svn commit: r1836239 - in /httpd/httpd/trunk: include/httpd.h modules/http/http_request.c server/mpm/event/event.c server/mpm/motorz/motorz.c server/mpm/simple/simple_io.c server/util_filter.c

[Ruediger Pluem]

On 07/20/2018 03:49 PM, Yann Ylavic wrote:
> On Fri, Jul 20, 2018 at 2:54 PM, Ruediger Pluem  wrote:
>>
>> On 07/20/2018 02:38 PM, Yann Ylavic wrote:
>>> On Fri, Jul 20, 2018 at 12:08 PM, Ruediger Pluem  wrote:

 On 07/19/2018 12:36 AM, yla...@apache.org wrote:
>
> -if (!c->data_in_input_filters || ap_run_input_pending(c) != OK) {
> +if (ap_run_input_pending(c) != OK) {

 We have a different code flow here now. If c->data_in_input_filters is 0, 
 then
 ap_filter_input_pending does iterate over the ring whereas it did not do 
 before,
 because it was not called.
>>>
>>> Right, though ap_filter_input_pending() iteration is quite cheap, and
>>> could be even cheaper if pending input and output filters were handled
>>> separetely (next step...).
>>>
>>> I prefer to keep the logic in ap_filter_*_pending() if you don't mind,
>>> and avoid global c->data_in_*_filter checks all over the place
>>> (possibly they'll disappear from conn_rec some day).
>>> For now c->data_in_*_filter are used internally (core) to positively
>>> force pending data, never negatively (and that's wise I think).
>>
>> So if c->data_in_input_filters is 0 the iteration is not expected to return 
>> OK,
>> as otherwise we would have an inconsistency between c->data_in_input_filters 
>> and the ring, correct?
> 
> Well, it's hard/unwise to keep a per-connection flag aligned with a
> per-filter feature, that's why those flags should disappear I think.
> But yes, when ap_run_input_pending() is called from
> ap_process_request() it will never return OK if
> c->data_in_input_filters is 0.
> 
> The issue, I think, is rather that if c->data_in_*_filters is 1 it
> will not change until the next call to ap_process_[async_]request(),
> even though the filter chain may be emptied until then, so
> ap_filter_*_pending() may return OK while it shouldn't.
> 
> Hmm, let me look at this more closely, it seems that these flags
> should really disappear now.
> 
>> On the other hand this inconsistency is possible if ap_run_input_pending is 
>> called from
>> other locations in the code than the above where we did not have this 
>> !c->data_in_input_filters check, correct?
> 
> This says the same thing as I said above right?

You describe the inconsistency in case c->data_in_input_filters is 1,
I did for the case that c->data_in_input_filters is 0, but yes we could have 
inconsistencies in both cases
and the question is if they are always fine. So far it looks like.
But probably the flags should just go in order to avoid these cases.

Regards

Rüdiger

Re: svn commit: r1836239 - in /httpd/httpd/trunk: include/httpd.h modules/http/http_request.c server/mpm/event/event.c server/mpm/motorz/motorz.c server/mpm/simple/simple_io.c server/util_filter.c

[Yann Ylavic]

On Fri, Jul 20, 2018 at 2:54 PM, Ruediger Pluem  wrote:
>
> On 07/20/2018 02:38 PM, Yann Ylavic wrote:
>> On Fri, Jul 20, 2018 at 12:08 PM, Ruediger Pluem  wrote:
>>>
>>> On 07/19/2018 12:36 AM, yla...@apache.org wrote:

 -if (!c->data_in_input_filters || ap_run_input_pending(c) != OK) {
 +if (ap_run_input_pending(c) != OK) {
>>>
>>> We have a different code flow here now. If c->data_in_input_filters is 0, 
>>> then
>>> ap_filter_input_pending does iterate over the ring whereas it did not do 
>>> before,
>>> because it was not called.
>>
>> Right, though ap_filter_input_pending() iteration is quite cheap, and
>> could be even cheaper if pending input and output filters were handled
>> separetely (next step...).
>>
>> I prefer to keep the logic in ap_filter_*_pending() if you don't mind,
>> and avoid global c->data_in_*_filter checks all over the place
>> (possibly they'll disappear from conn_rec some day).
>> For now c->data_in_*_filter are used internally (core) to positively
>> force pending data, never negatively (and that's wise I think).
>
> So if c->data_in_input_filters is 0 the iteration is not expected to return 
> OK,
> as otherwise we would have an inconsistency between c->data_in_input_filters 
> and the ring, correct?

Well, it's hard/unwise to keep a per-connection flag aligned with a
per-filter feature, that's why those flags should disappear I think.
But yes, when ap_run_input_pending() is called from
ap_process_request() it will never return OK if
c->data_in_input_filters is 0.

The issue, I think, is rather that if c->data_in_*_filters is 1 it
will not change until the next call to ap_process_[async_]request(),
even though the filter chain may be emptied until then, so
ap_filter_*_pending() may return OK while it shouldn't.

Hmm, let me look at this more closely, it seems that these flags
should really disappear now.

> On the other hand this inconsistency is possible if ap_run_input_pending is 
> called from
> other locations in the code than the above where we did not have this 
> !c->data_in_input_filters check, correct?

This says the same thing as I said above right?

Regards,
Yann.

Re: svn commit: r1832609 - in /httpd/httpd/branches/2.4.x: ./ docs/manual/howto/ docs/manual/mod/ modules/proxy/ modules/proxy/balancers/

[Yann Ylavic]

On Fri, Jul 20, 2018 at 2:56 PM, Ruediger Pluem  wrote:
>
> So maybe some checking is due there as well if the conditional fetching 
> should be removed?

+1 :)

Re: svn commit: r1834924 - /httpd/httpd/branches/2.4.x/STATUS

[Ruediger Pluem]

On 07/20/2018 02:49 PM, Yann Ylavic wrote:
> Ping, any objection if I commit this and add it to the backport proposal?

Hmm. Looks like MODSSL_ERROR_BAD_GATEWAY is only used when the proxy connects 
to the backend.
So the patch should be fine.

Regards

Rüdiger

> 
> On Tue, Jul 3, 2018 at 10:36 AM, Yann Ylavic  wrote:
>> On Tue, Jul 3, 2018 at 8:58 AM,   wrote:
>>>
>>> +++ httpd/httpd/branches/2.4.x/STATUS Tue Jul  3 06:58:55 2018
>>> @@ -179,7 +179,11 @@ PATCHES PROPOSED TO BACKPORT FROM TRUNK:
>>>   2.4.x patch: svn merge -c 1645529 ^/httpd/httpd/trunk .
>>>   +1: ylavic, druggeri
>>>   druggeri: Why no +1, jallietc36?
>>> -
>>> + rpluem: I think the patch is wrong here and in trunk. This causes
>>> + ap_pass_brigade to return APR_SUCCESS in ap_proxy_pass_brigade. The 
>>> error
>>> + bucket inserted by ssl_io_filter_error IMHO makes no sense because it
>>> + would be sent to the origin server (the proxy backend) and not to our
>>> + client. Further discussion should possibly happen on dev@.
>>
>> Agreed, but r1645529 looks right to me, I'd rather fix
>> ssl_io_filter_error() to return EGENERAL (and no error brigade) in
>> this case.
>>
>> Something like this:
>> Index: modules/ssl/ssl_engine_io.c
>> ===
>> --- modules/ssl/ssl_engine_io.c(revision 1834106)
>> +++ modules/ssl/ssl_engine_io.c(working copy)
>> @@ -1008,14 +1008,10 @@ static apr_status_t ssl_io_filter_error(bio_filter
>>  break;
>>
>>  case MODSSL_ERROR_BAD_GATEWAY:
>> -/* Send an error bucket, though the proxy currently has no
>> - * special handling for error buckets and ignores this. */
>> -bucket = ap_bucket_error_create(HTTP_BAD_GATEWAY, NULL,
>> -f->c->pool,
>> -f->c->bucket_alloc);
>>  ap_log_cerror(APLOG_MARK, APLOG_INFO, 0, f->c, APLOGNO(01997)
>>"SSL handshake failed: sending 502");
>> -break;
>> +f->c->aborted = 1;
>> +return APR_EGENERAL;
>>
>>  default:
>>  return status;
>> ?
> 

Re: svn commit: r1832609 - in /httpd/httpd/branches/2.4.x: ./ docs/manual/howto/ docs/manual/mod/ modules/proxy/ modules/proxy/balancers/

[Ruediger Pluem]

On 07/20/2018 02:45 PM, Yann Ylavic wrote:
> On Fri, Jul 20, 2018 at 12:13 PM, Ruediger Pluem  wrote:
>>
>> Something like the attached? The mod_lbmethod_byrequests.c part needs to be 
>> done for lb modules though.
> 
> +/* post_config hook: */
> +static int lbmethod_byrequests_post_config(apr_pool_t *pconf, apr_pool_t 
> *plog,
> +apr_pool_t *ptemp, server_rec *s)
> +{
> +
> +/* lbmethod_byrequests_post_config() will be called twice during
> startup.  So, don't
> + * set up the static data the 1st time through. */
> +if (ap_state_query(AP_SQ_MAIN_STATE) == AP_SQ_MS_CREATE_PRE_CONFIG) {
> +return OK;
> +}
> +
> +if (!ap_proxy_balancer_get_best_worker_fn) {
> 
> Shouldn't we remove this check to retrieve the function pointer 
> unconditionally?
> With static linking the pointer may not be NULL but still point to
> garbage (mod_lbmethod_byrequests linked statically but mod_proxy
> linked dynamically, if that's ever possible). APR_RETRIEVE_OPTIONAL_FN
> is cheap anyway.

Good point. But I just followed the same pattern as other code in mod_proxy 
does :-).
So maybe some checking is due there as well if the conditional fetching should 
be removed?

Regards

Rüdiger

Re: svn commit: r1836239 - in /httpd/httpd/trunk: include/httpd.h modules/http/http_request.c server/mpm/event/event.c server/mpm/motorz/motorz.c server/mpm/simple/simple_io.c server/util_filter.c

[Ruediger Pluem]

On 07/20/2018 02:38 PM, Yann Ylavic wrote:
> On Fri, Jul 20, 2018 at 12:08 PM, Ruediger Pluem  wrote:
>>
>> On 07/19/2018 12:36 AM, yla...@apache.org wrote:
>>>
>>> -if (!c->data_in_input_filters || ap_run_input_pending(c) != OK) {
>>> +if (ap_run_input_pending(c) != OK) {
>>
>> We have a different code flow here now. If c->data_in_input_filters is 0, 
>> then
>> ap_filter_input_pending does iterate over the ring whereas it did not do 
>> before,
>> because it was not called.
> 
> Right, though ap_filter_input_pending() iteration is quite cheap, and
> could be even cheaper if pending input and output filters were handled
> separetely (next step...).
> 
> I prefer to keep the logic in ap_filter_*_pending() if you don't mind,
> and avoid global c->data_in_*_filter checks all over the place
> (possibly they'll disappear from conn_rec some day).
> For now c->data_in_*_filter are used internally (core) to positively
> force pending data, never negatively (and that's wise I think).

So if c->data_in_input_filters is 0 the iteration is not expected to return OK,
as otherwise we would have an inconsistency between c->data_in_input_filters 
and the ring, correct?
On the other hand this inconsistency is possible if ap_run_input_pending is 
called from
other locations in the code than the above where we did not have this 
!c->data_in_input_filters check, correct?

Regards

Rüdiger

Re: svn commit: r1834924 - /httpd/httpd/branches/2.4.x/STATUS

[Yann Ylavic]

Ping, any objection if I commit this and add it to the backport proposal?

On Tue, Jul 3, 2018 at 10:36 AM, Yann Ylavic  wrote:
> On Tue, Jul 3, 2018 at 8:58 AM,   wrote:
>>
>> +++ httpd/httpd/branches/2.4.x/STATUS Tue Jul  3 06:58:55 2018
>> @@ -179,7 +179,11 @@ PATCHES PROPOSED TO BACKPORT FROM TRUNK:
>>   2.4.x patch: svn merge -c 1645529 ^/httpd/httpd/trunk .
>>   +1: ylavic, druggeri
>>   druggeri: Why no +1, jallietc36?
>> -
>> + rpluem: I think the patch is wrong here and in trunk. This causes
>> + ap_pass_brigade to return APR_SUCCESS in ap_proxy_pass_brigade. The 
>> error
>> + bucket inserted by ssl_io_filter_error IMHO makes no sense because it
>> + would be sent to the origin server (the proxy backend) and not to our
>> + client. Further discussion should possibly happen on dev@.
>
> Agreed, but r1645529 looks right to me, I'd rather fix
> ssl_io_filter_error() to return EGENERAL (and no error brigade) in
> this case.
>
> Something like this:
> Index: modules/ssl/ssl_engine_io.c
> ===
> --- modules/ssl/ssl_engine_io.c(revision 1834106)
> +++ modules/ssl/ssl_engine_io.c(working copy)
> @@ -1008,14 +1008,10 @@ static apr_status_t ssl_io_filter_error(bio_filter
>  break;
>
>  case MODSSL_ERROR_BAD_GATEWAY:
> -/* Send an error bucket, though the proxy currently has no
> - * special handling for error buckets and ignores this. */
> -bucket = ap_bucket_error_create(HTTP_BAD_GATEWAY, NULL,
> -f->c->pool,
> -f->c->bucket_alloc);
>  ap_log_cerror(APLOG_MARK, APLOG_INFO, 0, f->c, APLOGNO(01997)
>"SSL handshake failed: sending 502");
> -break;
> +f->c->aborted = 1;
> +return APR_EGENERAL;
>
>  default:
>  return status;
> ?

Re: svn commit: r1832609 - in /httpd/httpd/branches/2.4.x: ./ docs/manual/howto/ docs/manual/mod/ modules/proxy/ modules/proxy/balancers/

[Yann Ylavic]

On Fri, Jul 20, 2018 at 12:13 PM, Ruediger Pluem  wrote:
>
> Something like the attached? The mod_lbmethod_byrequests.c part needs to be 
> done for lb modules though.

+/* post_config hook: */
+static int lbmethod_byrequests_post_config(apr_pool_t *pconf, apr_pool_t *plog,
+apr_pool_t *ptemp, server_rec *s)
+{
+
+/* lbmethod_byrequests_post_config() will be called twice during
startup.  So, don't
+ * set up the static data the 1st time through. */
+if (ap_state_query(AP_SQ_MAIN_STATE) == AP_SQ_MS_CREATE_PRE_CONFIG) {
+return OK;
+}
+
+if (!ap_proxy_balancer_get_best_worker_fn) {

Shouldn't we remove this check to retrieve the function pointer unconditionally?
With static linking the pointer may not be NULL but still point to
garbage (mod_lbmethod_byrequests linked statically but mod_proxy
linked dynamically, if that's ever possible). APR_RETRIEVE_OPTIONAL_FN
is cheap anyway.

+ap_proxy_balancer_get_best_worker_fn =
+APR_RETRIEVE_OPTIONAL_FN(ap_proxy_balancer_get_best_worker);
+if (!ap_proxy_balancer_get_best_worker_fn) {
+ap_log_error(APLOG_MARK, APLOG_EMERG, 0, s, APLOGNO()
+ "mod_proxy must be loaded for
mod_lbmethod_byrequests");
+return !OK;
+}
+}
+
+return OK;
+}

Otherwise, the patch looks good to me, thanks!

Regards,
Yann.

Re: svn commit: r1836239 - in /httpd/httpd/trunk: include/httpd.h modules/http/http_request.c server/mpm/event/event.c server/mpm/motorz/motorz.c server/mpm/simple/simple_io.c server/util_filter.c

[Yann Ylavic]

On Fri, Jul 20, 2018 at 12:08 PM, Ruediger Pluem  wrote:
>
> On 07/19/2018 12:36 AM, yla...@apache.org wrote:
>>
>> -if (!c->data_in_input_filters || ap_run_input_pending(c) != OK) {
>> +if (ap_run_input_pending(c) != OK) {
>
> We have a different code flow here now. If c->data_in_input_filters is 0, then
> ap_filter_input_pending does iterate over the ring whereas it did not do 
> before,
> because it was not called.

Right, though ap_filter_input_pending() iteration is quite cheap, and
could be even cheaper if pending input and output filters were handled
separetely (next step...).

I prefer to keep the logic in ap_filter_*_pending() if you don't mind,
and avoid global c->data_in_*_filter checks all over the place
(possibly they'll disappear from conn_rec some day).
For now c->data_in_*_filter are used internally (core) to positively
force pending data, never negatively (and that's wise I think).

Regards,
Yann.

Re: Bug in mod_ratelimit?

[Yann Ylavic]

Hi Cory,

On Thu, Jul 19, 2018 at 11:23 PM, Cory McIntire  wrote:
>
> We’re going to revert to the 2.4.33 version of mod_ratelimit for now.
>
> HEAD requests with large amount of headers were still problematic in our 
> testing with both versions of the patch applied.

Thanks for letting us know.

I think the right fix is the attached patch (tested with GET/HEAD with
large header and/or body, seems to work).
If by any chance you can give it a try...

@team: the issue with HEAD request is that ap_http_header_filter()
eats the final EOS (along with the body), I don't think this is
correct because downstream filters may depend on it to bail out (like
rate_limit_filter() does now).
So how about the attached patch both with regard to level
FTYPE_CONNECTION - 1 for ratelimit filter and ap_http_header_filter()
to forward the final EOS?

Regards,
Yann.
Index: modules/filters/mod_ratelimit.c
===
--- modules/filters/mod_ratelimit.c	(revision 1836337)
+++ modules/filters/mod_ratelimit.c	(working copy)
@@ -327,7 +327,7 @@ static void register_hooks(apr_pool_t *p)
 {
 /* run after mod_deflate etc etc, but not at connection level, ie, mod_ssl. */
 ap_register_output_filter(RATE_LIMIT_FILTER_NAME, rate_limit_filter,
-  NULL, AP_FTYPE_PROTOCOL + 3);
+  NULL, AP_FTYPE_CONNECTION - 1);
 }
 
 AP_DECLARE_MODULE(ratelimit) = {
Index: modules/http/http_filters.c
===
--- modules/http/http_filters.c	(revision 1836337)
+++ modules/http/http_filters.c	(working copy)
@@ -1308,8 +1308,19 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_heade
 else if (ctx->headers_sent) {
 /* Eat body if response must not have one. */
 if (r->header_only || r->status == HTTP_NO_CONTENT) {
+/* Still next filters may be waiting for EOS, so pass it (alone)
+ * when encountered and be done with this filter.
+ */
+e = APR_BRIGADE_LAST(b);
+if (e != APR_BRIGADE_SENTINEL(b) && APR_BUCKET_IS_EOS(e)) {
+APR_BUCKET_REMOVE(e);
+apr_brigade_cleanup(b);
+APR_BRIGADE_INSERT_HEAD(b, e);
+ap_remove_output_filter(f);
+rv = ap_pass_brigade(f->next, b);
+}
 apr_brigade_cleanup(b);
-return APR_SUCCESS;
+return rv;
 }
 }
 

Re: svn commit: r1832609 - in /httpd/httpd/branches/2.4.x: ./ docs/manual/howto/ docs/manual/mod/ modules/proxy/ modules/proxy/balancers/

[Ruediger Pluem]

On 07/19/2018 08:24 PM, William A Rowe Jr wrote:
> On Thu, May 31, 2018 at 8:24 AM, mailto:j...@apache.org>> 
> wrote:
> 
> Author: jim
> Date: Thu May 31 13:24:04 2018
> New Revision: 1832609
> 
> URL: http://svn.apache.org/viewvc?rev=1832609=rev 
> 
> Log:
> Merge r1828890, r1832500 from trunk:
> 
> [...] 
> 
> URL:
> 
> http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/modules/proxy/balancers/mod_lbmethod_byrequests.c?rev=1832609=1832608=1832609=diff
> 
> 
> 
> ==
> --- 
> httpd/httpd/branches/2.4.x/modules/proxy/balancers/mod_lbmethod_byrequests.c 
> (original)
> +++ 
> httpd/httpd/branches/2.4.x/modules/proxy/balancers/mod_lbmethod_byrequests.c 
> Thu May 31 13:24:04 2018
> 
> [...] 
> 
> @@ -70,82 +77,17 @@ static int (*ap_proxy_retry_worker_fn)(c
>   *   b a d c d a c d b d ...
>   *
>   */
> -
>  static proxy_worker *find_best_byrequests(proxy_balancer *balancer,
>  request_rec *r)
>  {
> -int i;
>  int total_factor = 0;
> 
> [...] 
> 
> +proxy_worker *worker = ap_proxy_balancer_get_best_worker(balancer, 
> r, is_best_byrequests, _factor);
> 
> 
> This introduced a new hard runtime config ordering problem on 
> mod_lbmethod_byrequest.so, which must now be loaded AFTER
> mod_proxy.so.
> 
> This was not previously true, as illustrated by mod_lbmethod_heartbeat, using 
> the ap_proxy_retry_worker using an
> optional function.
> 
> lbmethod sorts before proxy, fwiw.
> 

Something like the attached? The mod_lbmethod_byrequests.c part needs to be 
done for lb modules though.

Regards

Rüdiger



Index: modules/proxy/proxy_util.c
===
--- modules/proxy/proxy_util.c	(revision 1836329)
+++ modules/proxy/proxy_util.c	(working copy)
@@ -4079,4 +4079,5 @@
 {
 APR_REGISTER_OPTIONAL_FN(ap_proxy_retry_worker);
 APR_REGISTER_OPTIONAL_FN(ap_proxy_clear_connection);
+APR_REGISTER_OPTIONAL_FN(ap_proxy_balancer_get_best_worker);
 }
Index: modules/proxy/balancers/mod_lbmethod_byrequests.c
===
--- modules/proxy/balancers/mod_lbmethod_byrequests.c	(revision 1836329)
+++ modules/proxy/balancers/mod_lbmethod_byrequests.c	(working copy)
@@ -22,6 +22,9 @@
 
 module AP_MODULE_DECLARE_DATA lbmethod_byrequests_module;
 
+static APR_OPTIONAL_FN_TYPE(ap_proxy_balancer_get_best_worker)
+*ap_proxy_balancer_get_best_worker_fn = NULL;
+
 static int is_best_byrequests(proxy_worker *current, proxy_worker *prev_best, void *baton)
 {
 int *total_factor = (int *)baton;
@@ -81,7 +84,7 @@
 request_rec *r)
 {
 int total_factor = 0;
-proxy_worker *worker = ap_proxy_balancer_get_best_worker(balancer, r, is_best_byrequests, _factor);
+proxy_worker *worker = ap_proxy_balancer_get_best_worker_fn(balancer, r, is_best_byrequests, _factor);
 
 if (worker) {
 worker->s->lbstatus -= total_factor;
@@ -123,6 +126,30 @@
 NULL
 };
 
+/* post_config hook: */
+static int lbmethod_byrequests_post_config(apr_pool_t *pconf, apr_pool_t *plog,
+apr_pool_t *ptemp, server_rec *s)
+{
+
+/* lbmethod_byrequests_post_config() will be called twice during startup.  So, don't
+ * set up the static data the 1st time through. */
+if (ap_state_query(AP_SQ_MAIN_STATE) == AP_SQ_MS_CREATE_PRE_CONFIG) {
+return OK;
+}
+
+if (!ap_proxy_balancer_get_best_worker_fn) {
+ap_proxy_balancer_get_best_worker_fn =
+APR_RETRIEVE_OPTIONAL_FN(ap_proxy_balancer_get_best_worker);
+if (!ap_proxy_balancer_get_best_worker_fn) {
+ap_log_error(APLOG_MARK, APLOG_EMERG, 0, s, APLOGNO()
+ "mod_proxy must be loaded for mod_lbmethod_byrequests");
+return !OK;
+}
+}
+
+return OK;
+}
+
 static void register_hook(apr_pool_t *p)
 {
 /* Only the mpm_winnt has child init hook handler.
@@ -130,6 +157,7 @@
  * initializes and after the mod_proxy
  */
 ap_register_provider(p, PROXY_LBMETHOD, "byrequests", "0", );
+ap_hook_post_config(lbmethod_byrequests_post_config, NULL, NULL, APR_HOOK_MIDDLE);
 }
 
 AP_DECLARE_MODULE(lbmethod_byrequests) = {
Index: modules/proxy/mod_proxy.h
===
--- modules/proxy/mod_proxy.h	(revision 1836329)
+++ modules/proxy/mod_proxy.h	(working copy)
@@ -880,6 +880,14 @@
 request_rec *r,
 proxy_is_best_callback_fn_t *is_best,
 

Re: svn commit: r1836239 - in /httpd/httpd/trunk: include/httpd.h modules/http/http_request.c server/mpm/event/event.c server/mpm/motorz/motorz.c server/mpm/simple/simple_io.c server/util_filter.c

[Ruediger Pluem]

On 07/19/2018 12:36 AM, yla...@apache.org wrote:
> Author: ylavic
> Date: Wed Jul 18 22:36:19 2018
> New Revision: 1836239
> 
> URL: http://svn.apache.org/viewvc?rev=1836239=rev
> Log:
> core: integrate data_in_{in,out}put_filter to ap_filter_{in,out}put_pending().
> 
> Straightforward for ap_filter_input_pending() since c->data_in_input_filter is
> always checked wherever ap_run_input_pending(c) is.
> 
> For ap_filter_input_pending(), it allows to set c->data_in_output_filter in
> ap_process_request_after_handler() to avoid an useless flush from mpm_event.
> 
> Modified:
> httpd/httpd/trunk/include/httpd.h
> httpd/httpd/trunk/modules/http/http_request.c
> httpd/httpd/trunk/server/mpm/event/event.c
> httpd/httpd/trunk/server/mpm/motorz/motorz.c
> httpd/httpd/trunk/server/mpm/simple/simple_io.c
> httpd/httpd/trunk/server/util_filter.c
> 

> Modified: httpd/httpd/trunk/modules/http/http_request.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http/http_request.c?rev=1836239=1836238=1836239=diff
> ==
> --- httpd/httpd/trunk/modules/http/http_request.c (original)
> +++ httpd/httpd/trunk/modules/http/http_request.c Wed Jul 18 22:36:19 2018
> @@ -403,9 +403,20 @@ AP_DECLARE(void) ap_process_request_afte
>  c->data_in_input_filters = (rv == APR_SUCCESS);
>  apr_brigade_cleanup(bb);
>  
> -if (c->cs)
> -c->cs->state = (c->aborted) ? CONN_STATE_LINGER
> -: CONN_STATE_WRITE_COMPLETION;
> +if (c->cs) {
> +if (c->aborted) {
> +c->cs->state = CONN_STATE_LINGER;
> +}
> +else {
> +/* If we have still data in the output filters here it means that
> + * the last (recent) nonblocking write was EAGAIN, so tell the 
> MPM
> + * to not try another useless/stressful one but to go straight to
> + * POLLOUT.
> +*/
> +c->data_in_output_filters = 
> ap_filter_should_yield(c->output_filters);
> +c->cs->state = CONN_STATE_WRITE_COMPLETION;
> +}
> +}
>  AP_PROCESS_REQUEST_RETURN((uintptr_t)r, r->uri, r->status);
>  if (ap_extended_status) {
>  ap_time_process_request(c->sbh, STOP_PREQUEST);
> @@ -494,7 +505,7 @@ AP_DECLARE(void) ap_process_request(requ
>  
>  ap_process_async_request(r);
>  
> -if (!c->data_in_input_filters || ap_run_input_pending(c) != OK) {
> +if (ap_run_input_pending(c) != OK) {

We have a different code flow here now. If c->data_in_input_filters is 0, then
ap_filter_input_pending does iterate over the ring whereas it did not do before,
because it was not called.


>  bb = ap_reuse_brigade_from_pool("ap_pr_bb", c->pool, 
> c->bucket_alloc);
>  b = apr_bucket_flush_create(c->bucket_alloc);
>  APR_BRIGADE_INSERT_HEAD(bb, b);
> 

Regards

Rüdiger

[RESOLVED] Re: Regression with CMake Windows builds, 2.4.29 is the last one passing

[Michal Karm]

On 07/19/2018 08:18 PM, William A Rowe Jr wrote:
> Hi Michal, thanks for your report.
>
> On Thu, Jul 19, 2018 at 10:36 AM, Michal Karm  > wrote:
>
> Hi guys,
>
> I would like to ask whether there are any plans on
> accepting [1] patch to 2.4.x as it keeps failing the CMake Windows build.
>
> Furthermore, there is a regression [2] in 2.4.34 CMake Windows build that
> did not
> exist in 2.4.29.
>
> If anyone has a hit or an idea handy to get me unstuck I try to come up 
> with a
> patch.
>
> Thank you for your time
> K.
>
> [1] https://bz.apache.org/bugzilla/show_bug.cgi?id=62190
> 
>
>
> Suggestion adopted, as should have happened in the original backport. The
> changes are now merged complete from trunk.
>  
>
> [2] https://bz.apache.org/bugzilla/show_bug.cgi?id=62557
> 
>
>
> Proposed a solution for you. If this builds clean for you, I'll commit to
> trunk and 2.4. 

Thank you for the hint. Updated patches are on the Bugzilla, including links to
passing builds.



Michal Karm Babacek

-- 
Sent from my Hosaka Ono-Sendai Cyberspace 7

Re: AW: ocsp_force_default initialized with UNSET in httpd 2.4.34

[Ruediger Pluem]

Proposed for backport as r1836334.

Regards

Rüdiger

On 07/19/2018 11:23 AM, Plüm, Rüdiger, Vodafone Group wrote:
> I can do tomorrow and make a proposal in STATUS. Looks like we are all 
> aligned now how to resolve this.
> 
> Regards
> 
> Rüdiger
> 
>> -Ursprüngliche Nachricht-
>> Von: Stefan Eissing 
>> Gesendet: Donnerstag, 19. Juli 2018 11:10
>> An: dev@httpd.apache.org
>> Betreff: Re: ocsp_force_default initialized with UNSET in httpd 2.4.34
>>
>> You'll take care of it, Rüdiger?
>>
>>> Am 18.07.2018 um 13:57 schrieb Ruediger Pluem :
>>>
>>>
>>>
>>> On 07/18/2018 11:44 AM, Stefan Eissing wrote:
> Am 18.07.2018 um 11:37 schrieb Yann Ylavic :
>
> On Wed, Jul 18, 2018 at 11:29 AM, Ruediger Pluem 
>> wrote:
>>
>> Good catch. Maybe a dirty working copy during backport? Yann?
>
> Actually the change was in the proposed patch:
> https://svn.apache.org/repos/asf/httpd/httpd/patches/2.4.x/ssl-ocsp-
>> enable-leaf.patch
> A subtle difference between trunk and 2.4.x, around the change...

 Hmm, so I had the dirty working dir when creating the patch? I do not
>> remember messing with that setting, but obviously I was mistaken in
>> doing it.

 So, patch 1 it is then, Rainer?

>>>
>>> I changed my mind :-) Let's backport r1555631 from trunk which is more
>> or less patch 2. So we have aligned trunk and
>>> 2.4.x here. r1555631 does not apply clean though, because r1826995,
>> r1827001 are already backported, but this is fixable.
>>>
>>> Regards
>>>
>>> Rüdiger
> 

Re: ocsp_force_default initialized with UNSET in httpd 2.4.34

[Frank Meier]

On 18/07/18 13:57, Ruediger Pluem wrote:


On 07/18/2018 11:44 AM, Stefan Eissing wrote:

Am 18.07.2018 um 11:37 schrieb Yann Ylavic :

On Wed, Jul 18, 2018 at 11:29 AM, Ruediger Pluem  wrote:

Good catch. Maybe a dirty working copy during backport? Yann?

Actually the change was in the proposed patch:
https://svn.apache.org/repos/asf/httpd/httpd/patches/2.4.x/ssl-ocsp-enable-leaf.patch
A subtle difference between trunk and 2.4.x, around the change...

Hmm, so I had the dirty working dir when creating the patch? I do not remember 
messing with that setting, but obviously I was mistaken in doing it.

So, patch 1 it is then, Rainer?


I changed my mind :-) Let's backport r1555631 from trunk which is more or less 
patch 2. So we have aligned trunk and
2.4.x here. r1555631 does not apply clean though, because r1826995, r1827001 
are already backported, but this is fixable.

Regards

Rüdiger

Thanks Guys, so we will run with patch 2. For now.
Just for other people to know if they hit this issue with 2.4.34 and are 
not able to patch, there is a workaround: Explicitly setting the 
directive "SSLOCSPOverrideResponder" to "off" in the configuration file 
does also "fix" the issue.


Cheers, Frank