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
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: 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
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
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: 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
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: 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
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: 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
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
