Mailing List Archive

Re: svn commit: r1862475 - in /httpd/httpd/trunk: CHANGES modules/http2/h2_conn.c modules/http2/h2_version.h server/mpm/event/event.c
When looking at this again while researching something I couldn't answer myself the questions below
and as event mpm and mod_http2 are sometimes pretty complex I thought I ask :-)

On 7/3/19 3:46 PM, icing@apache.org wrote:
> Author: icing
> Date: Wed Jul 3 13:46:31 2019
> New Revision: 1862475
>
> URL: http://svn.apache.org/viewvc?rev=1862475&view=rev
> Log:
> *) mod_http2/mpm_event: Fixes the behaviour when a HTTP/2 connection has nothing
> more to write with streams ongoing (flow control block). The timeout waiting
> for the client to send WINODW_UPDATE was incorrectly KeepAliveTimeout and not
> Timeout as it should be. Fixes PR 63534. [Yann Ylavic, Stefan Eissing]
>
>
> Modified:
> httpd/httpd/trunk/CHANGES
> httpd/httpd/trunk/modules/http2/h2_conn.c
> httpd/httpd/trunk/modules/http2/h2_version.h
> httpd/httpd/trunk/server/mpm/event/event.c
>

> Modified: httpd/httpd/trunk/modules/http2/h2_conn.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_conn.c?rev=1862475&r1=1862474&r2=1862475&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/http2/h2_conn.c (original)
> +++ httpd/httpd/trunk/modules/http2/h2_conn.c Wed Jul 3 13:46:31 2019
> @@ -231,6 +231,13 @@ apr_status_t h2_conn_run(conn_rec *c)
> case H2_SESSION_ST_BUSY:
> case H2_SESSION_ST_WAIT:
> c->cs->state = CONN_STATE_WRITE_COMPLETION;
> + if (c->cs && (session->open_streams || !session->remote.emitted_count)) {
> + /* let the MPM know that we are not done and want
> + * the Timeout behaviour instead of a KeepAliveTimeout
> + * See PR 63534.
> + */
> + c->cs->sense = CONN_SENSE_WANT_READ;

Can we get here with session->open_streams > 0 and all the open streams are only producing output and none of them is waiting for
something from the client?


> + }
> break;
> case H2_SESSION_ST_CLEANUP:
> case H2_SESSION_ST_DONE:
>

> Modified: httpd/httpd/trunk/server/mpm/event/event.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/mpm/event/event.c?rev=1862475&r1=1862474&r2=1862475&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/server/mpm/event/event.c (original)
> +++ httpd/httpd/trunk/server/mpm/event/event.c Wed Jul 3 13:46:31 2019
> @@ -1153,10 +1153,11 @@ read_request:
> else if (ap_filter_should_yield(c->output_filters)) {
> pending = OK;
> }
> - if (pending == OK) {
> + if (pending == OK || (pending == DECLINED &&
> + cs->pub.sense == CONN_SENSE_WANT_READ)) {
> /* Still in WRITE_COMPLETION_STATE:
> - * Set a write timeout for this connection, and let the
> - * event thread poll for writeability.
> + * Set a read/write timeout for this connection, and let the
> + * event thread poll for read/writeability.
> */
> cs->queue_timestamp = apr_time_now();
> notify_suspend(cs);

Hm. Showing following code lines from trunk for my next question:


update_reqevents_from_sense(cs, -1);

if cs->pub.sense == CONN_SENSE_WANT_READ we subscribe on to POLLIN only on the pfd


apr_thread_mutex_lock(timeout_mutex);
TO_QUEUE_APPEND(cs->sc->wc_q, cs);
rv = apr_pollset_add(event_pollset, &cs->pfd);

If the read event triggers we will get back to the

if (cs->pub.state == CONN_STATE_WRITE_COMPLETION) {
int pending = DECLINED;

above. This means we try flush the output queue if it is not empty and if it is empty after this we would fall through to
set cs->pub.state = CONN_STATE_CHECK_REQUEST_LINE_READABLE; and then add the pfd to the pollset again and poll again. This should
trigger the read event again and we would process the input. But if I see this correctly we would need to poll twice for getting
the read data processed.
OTOH if we do not manage to clear the output queue above we would add the pfd to the pollset again but this time for writing and
only once all output has been processed we would take care of the reading.
Maybe we should take care of both?


Regards

Rüdiger
Re: svn commit: r1862475 - in /httpd/httpd/trunk: CHANGES modules/http2/h2_conn.c modules/http2/h2_version.h server/mpm/event/event.c [ In reply to ]
Things have been quite busy here, because of the release. Hence I think it's worth asking again :-).

Regards

Rüdiger

On 9/14/21 1:43 PM, Ruediger Pluem wrote:
> When looking at this again while researching something I couldn't answer myself the questions below
> and as event mpm and mod_http2 are sometimes pretty complex I thought I ask :-)
>
> On 7/3/19 3:46 PM, icing@apache.org wrote:
>> Author: icing
>> Date: Wed Jul 3 13:46:31 2019
>> New Revision: 1862475
>>
>> URL: http://svn.apache.org/viewvc?rev=1862475&view=rev
>> Log:
>> *) mod_http2/mpm_event: Fixes the behaviour when a HTTP/2 connection has nothing
>> more to write with streams ongoing (flow control block). The timeout waiting
>> for the client to send WINODW_UPDATE was incorrectly KeepAliveTimeout and not
>> Timeout as it should be. Fixes PR 63534. [Yann Ylavic, Stefan Eissing]
>>
>>
>> Modified:
>> httpd/httpd/trunk/CHANGES
>> httpd/httpd/trunk/modules/http2/h2_conn.c
>> httpd/httpd/trunk/modules/http2/h2_version.h
>> httpd/httpd/trunk/server/mpm/event/event.c
>>
>
>> Modified: httpd/httpd/trunk/modules/http2/h2_conn.c
>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_conn.c?rev=1862475&r1=1862474&r2=1862475&view=diff
>> ==============================================================================
>> --- httpd/httpd/trunk/modules/http2/h2_conn.c (original)
>> +++ httpd/httpd/trunk/modules/http2/h2_conn.c Wed Jul 3 13:46:31 2019
>> @@ -231,6 +231,13 @@ apr_status_t h2_conn_run(conn_rec *c)
>> case H2_SESSION_ST_BUSY:
>> case H2_SESSION_ST_WAIT:
>> c->cs->state = CONN_STATE_WRITE_COMPLETION;
>> + if (c->cs && (session->open_streams || !session->remote.emitted_count)) {
>> + /* let the MPM know that we are not done and want
>> + * the Timeout behaviour instead of a KeepAliveTimeout
>> + * See PR 63534.
>> + */
>> + c->cs->sense = CONN_SENSE_WANT_READ;
>
> Can we get here with session->open_streams > 0 and all the open streams are only producing output and none of them is waiting for
> something from the client?
>
>
>> + }
>> break;
>> case H2_SESSION_ST_CLEANUP:
>> case H2_SESSION_ST_DONE:
>>
>
>> Modified: httpd/httpd/trunk/server/mpm/event/event.c
>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/mpm/event/event.c?rev=1862475&r1=1862474&r2=1862475&view=diff
>> ==============================================================================
>> --- httpd/httpd/trunk/server/mpm/event/event.c (original)
>> +++ httpd/httpd/trunk/server/mpm/event/event.c Wed Jul 3 13:46:31 2019
>> @@ -1153,10 +1153,11 @@ read_request:
>> else if (ap_filter_should_yield(c->output_filters)) {
>> pending = OK;
>> }
>> - if (pending == OK) {
>> + if (pending == OK || (pending == DECLINED &&
>> + cs->pub.sense == CONN_SENSE_WANT_READ)) {
>> /* Still in WRITE_COMPLETION_STATE:
>> - * Set a write timeout for this connection, and let the
>> - * event thread poll for writeability.
>> + * Set a read/write timeout for this connection, and let the
>> + * event thread poll for read/writeability.
>> */
>> cs->queue_timestamp = apr_time_now();
>> notify_suspend(cs);
>
> Hm. Showing following code lines from trunk for my next question:
>
>
> update_reqevents_from_sense(cs, -1);
>
> if cs->pub.sense == CONN_SENSE_WANT_READ we subscribe on to POLLIN only on the pfd
>
>
> apr_thread_mutex_lock(timeout_mutex);
> TO_QUEUE_APPEND(cs->sc->wc_q, cs);
> rv = apr_pollset_add(event_pollset, &cs->pfd);
>
> If the read event triggers we will get back to the
>
> if (cs->pub.state == CONN_STATE_WRITE_COMPLETION) {
> int pending = DECLINED;
>
> above. This means we try flush the output queue if it is not empty and if it is empty after this we would fall through to
> set cs->pub.state = CONN_STATE_CHECK_REQUEST_LINE_READABLE; and then add the pfd to the pollset again and poll again. This should
> trigger the read event again and we would process the input. But if I see this correctly we would need to poll twice for getting
> the read data processed.
> OTOH if we do not manage to clear the output queue above we would add the pfd to the pollset again but this time for writing and
> only once all output has been processed we would take care of the reading.
> Maybe we should take care of both?
>
>
> Regards
>
> Rüdiger
>
>
Re: svn commit: r1862475 - in /httpd/httpd/trunk: CHANGES modules/http2/h2_conn.c modules/http2/h2_version.h server/mpm/event/event.c [ In reply to ]
> Am 14.09.2021 um 13:43 schrieb Ruediger Pluem <rpluem@apache.org>:
>
> When looking at this again while researching something I couldn't answer myself the questions below
> and as event mpm and mod_http2 are sometimes pretty complex I thought I ask :-)
>
> On 7/3/19 3:46 PM, icing@apache.org wrote:
>> Author: icing
>> Date: Wed Jul 3 13:46:31 2019
>> New Revision: 1862475
>>
>> URL: http://svn.apache.org/viewvc?rev=1862475&view=rev
>> Log:
>> *) mod_http2/mpm_event: Fixes the behaviour when a HTTP/2 connection has nothing
>> more to write with streams ongoing (flow control block). The timeout waiting
>> for the client to send WINODW_UPDATE was incorrectly KeepAliveTimeout and not
>> Timeout as it should be. Fixes PR 63534. [Yann Ylavic, Stefan Eissing]
>>
>>
>> Modified:
>> httpd/httpd/trunk/CHANGES
>> httpd/httpd/trunk/modules/http2/h2_conn.c
>> httpd/httpd/trunk/modules/http2/h2_version.h
>> httpd/httpd/trunk/server/mpm/event/event.c
>>
>
>> Modified: httpd/httpd/trunk/modules/http2/h2_conn.c
>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_conn.c?rev=1862475&r1=1862474&r2=1862475&view=diff
>> ==============================================================================
>> --- httpd/httpd/trunk/modules/http2/h2_conn.c (original)
>> +++ httpd/httpd/trunk/modules/http2/h2_conn.c Wed Jul 3 13:46:31 2019
>> @@ -231,6 +231,13 @@ apr_status_t h2_conn_run(conn_rec *c)
>> case H2_SESSION_ST_BUSY:
>> case H2_SESSION_ST_WAIT:
>> c->cs->state = CONN_STATE_WRITE_COMPLETION;
>> + if (c->cs && (session->open_streams || !session->remote.emitted_count)) {
>> + /* let the MPM know that we are not done and want
>> + * the Timeout behaviour instead of a KeepAliveTimeout
>> + * See PR 63534.
>> + */
>> + c->cs->sense = CONN_SENSE_WANT_READ;
>
> Can we get here with session->open_streams > 0 and all the open streams are only producing output and none of them is waiting for
> something from the client?

Yes, but only when the HTTP/2 flow control blocks any progress. New data has to arrive from the client before the server is able to send more. So, we are waiting for new data from the client in the form of window updates.

>> + }
>> break;
>> case H2_SESSION_ST_CLEANUP:
>> case H2_SESSION_ST_DONE:
>>
>
>> Modified: httpd/httpd/trunk/server/mpm/event/event.c
>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/mpm/event/event.c?rev=1862475&r1=1862474&r2=1862475&view=diff
>> ==============================================================================
>> --- httpd/httpd/trunk/server/mpm/event/event.c (original)
>> +++ httpd/httpd/trunk/server/mpm/event/event.c Wed Jul 3 13:46:31 2019
>> @@ -1153,10 +1153,11 @@ read_request:
>> else if (ap_filter_should_yield(c->output_filters)) {
>> pending = OK;
>> }
>> - if (pending == OK) {
>> + if (pending == OK || (pending == DECLINED &&
>> + cs->pub.sense == CONN_SENSE_WANT_READ)) {
>> /* Still in WRITE_COMPLETION_STATE:
>> - * Set a write timeout for this connection, and let the
>> - * event thread poll for writeability.
>> + * Set a read/write timeout for this connection, and let the
>> + * event thread poll for read/writeability.
>> */
>> cs->queue_timestamp = apr_time_now();
>> notify_suspend(cs);
>
> Hm. Showing following code lines from trunk for my next question:
>
>
> update_reqevents_from_sense(cs, -1);
>
> if cs->pub.sense == CONN_SENSE_WANT_READ we subscribe on to POLLIN only on the pfd
>
>
> apr_thread_mutex_lock(timeout_mutex);
> TO_QUEUE_APPEND(cs->sc->wc_q, cs);
> rv = apr_pollset_add(event_pollset, &cs->pfd);
>
> If the read event triggers we will get back to the
>
> if (cs->pub.state == CONN_STATE_WRITE_COMPLETION) {
> int pending = DECLINED;
>
> above. This means we try flush the output queue if it is not empty and if it is empty after this we would fall through to
> set cs->pub.state = CONN_STATE_CHECK_REQUEST_LINE_READABLE; and then add the pfd to the pollset again and poll again. This should
> trigger the read event again and we would process the input. But if I see this correctly we would need to poll twice for getting
> the read data processed.
> OTOH if we do not manage to clear the output queue above we would add the pfd to the pollset again but this time for writing and
> only once all output has been processed we would take care of the reading.
> Maybe we should take care of both?

Hmm, but can it? In my understanding CONN_STATE_WRITE_COMPLETION is intended to flush the out buffers before putting in new work.

Since conn_rec's are limited to be processes in one thread-at-a-time only, POLLIN would call process_connection and that alone would block any more processing of the connection's output. Or?

Cheers, Stefan
Re: svn commit: r1862475 - in /httpd/httpd/trunk: CHANGES modules/http2/h2_conn.c modules/http2/h2_version.h server/mpm/event/event.c [ In reply to ]
> Am 20.09.2021 um 11:17 schrieb stefan@eissing.org:
>
>
>
>> Am 14.09.2021 um 13:43 schrieb Ruediger Pluem <rpluem@apache.org>:
>>
>> When looking at this again while researching something I couldn't answer myself the questions below
>> and as event mpm and mod_http2 are sometimes pretty complex I thought I ask :-)
>>
>> On 7/3/19 3:46 PM, icing@apache.org wrote:
>>> Author: icing
>>> Date: Wed Jul 3 13:46:31 2019
>>> New Revision: 1862475
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1862475&view=rev
>>> Log:
>>> *) mod_http2/mpm_event: Fixes the behaviour when a HTTP/2 connection has nothing
>>> more to write with streams ongoing (flow control block). The timeout waiting
>>> for the client to send WINODW_UPDATE was incorrectly KeepAliveTimeout and not
>>> Timeout as it should be. Fixes PR 63534. [Yann Ylavic, Stefan Eissing]
>>>
>>>
>>> Modified:
>>> httpd/httpd/trunk/CHANGES
>>> httpd/httpd/trunk/modules/http2/h2_conn.c
>>> httpd/httpd/trunk/modules/http2/h2_version.h
>>> httpd/httpd/trunk/server/mpm/event/event.c
>>>
>>
>>> Modified: httpd/httpd/trunk/modules/http2/h2_conn.c
>>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_conn.c?rev=1862475&r1=1862474&r2=1862475&view=diff
>>> ==============================================================================
>>> --- httpd/httpd/trunk/modules/http2/h2_conn.c (original)
>>> +++ httpd/httpd/trunk/modules/http2/h2_conn.c Wed Jul 3 13:46:31 2019
>>> @@ -231,6 +231,13 @@ apr_status_t h2_conn_run(conn_rec *c)
>>> case H2_SESSION_ST_BUSY:
>>> case H2_SESSION_ST_WAIT:
>>> c->cs->state = CONN_STATE_WRITE_COMPLETION;
>>> + if (c->cs && (session->open_streams || !session->remote.emitted_count)) {
>>> + /* let the MPM know that we are not done and want
>>> + * the Timeout behaviour instead of a KeepAliveTimeout
>>> + * See PR 63534.
>>> + */
>>> + c->cs->sense = CONN_SENSE_WANT_READ;
>>
>> Can we get here with session->open_streams > 0 and all the open streams are only producing output and none of them is waiting for
>> something from the client?
>
> Yes, but only when the HTTP/2 flow control blocks any progress. New data has to arrive from the client before the server is able to send more. So, we are waiting for new data from the client in the form of window updates.
>
>>> + }
>>> break;
>>> case H2_SESSION_ST_CLEANUP:
>>> case H2_SESSION_ST_DONE:
>>>
>>
>>> Modified: httpd/httpd/trunk/server/mpm/event/event.c
>>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/mpm/event/event.c?rev=1862475&r1=1862474&r2=1862475&view=diff
>>> ==============================================================================
>>> --- httpd/httpd/trunk/server/mpm/event/event.c (original)
>>> +++ httpd/httpd/trunk/server/mpm/event/event.c Wed Jul 3 13:46:31 2019
>>> @@ -1153,10 +1153,11 @@ read_request:
>>> else if (ap_filter_should_yield(c->output_filters)) {
>>> pending = OK;
>>> }
>>> - if (pending == OK) {
>>> + if (pending == OK || (pending == DECLINED &&
>>> + cs->pub.sense == CONN_SENSE_WANT_READ)) {
>>> /* Still in WRITE_COMPLETION_STATE:
>>> - * Set a write timeout for this connection, and let the
>>> - * event thread poll for writeability.
>>> + * Set a read/write timeout for this connection, and let the
>>> + * event thread poll for read/writeability.
>>> */
>>> cs->queue_timestamp = apr_time_now();
>>> notify_suspend(cs);
>>
>> Hm. Showing following code lines from trunk for my next question:
>>
>>
>> update_reqevents_from_sense(cs, -1);
>>
>> if cs->pub.sense == CONN_SENSE_WANT_READ we subscribe on to POLLIN only on the pfd
>>
>>
>> apr_thread_mutex_lock(timeout_mutex);
>> TO_QUEUE_APPEND(cs->sc->wc_q, cs);
>> rv = apr_pollset_add(event_pollset, &cs->pfd);
>>
>> If the read event triggers we will get back to the
>>
>> if (cs->pub.state == CONN_STATE_WRITE_COMPLETION) {
>> int pending = DECLINED;
>>
>> above. This means we try flush the output queue if it is not empty and if it is empty after this we would fall through to
>> set cs->pub.state = CONN_STATE_CHECK_REQUEST_LINE_READABLE; and then add the pfd to the pollset again and poll again. This should
>> trigger the read event again and we would process the input. But if I see this correctly we would need to poll twice for getting
>> the read data processed.
>> OTOH if we do not manage to clear the output queue above we would add the pfd to the pollset again but this time for writing and
>> only once all output has been processed we would take care of the reading.
>> Maybe we should take care of both?
>
> Hmm, but can it? In my understanding CONN_STATE_WRITE_COMPLETION is intended to flush the out buffers before putting in new work.
>
> Since conn_rec's are limited to be processes in one thread-at-a-time only, POLLIN would call process_connection and that alone would block any
*processed
> more processing of the connection's output. Or?
>
> Cheers, Stefan
Re: svn commit: r1862475 - in /httpd/httpd/trunk: CHANGES modules/http2/h2_conn.c modules/http2/h2_version.h server/mpm/event/event.c [ In reply to ]
On 9/20/21 11:17 AM, stefan@eissing.org wrote:
>
>
>> Am 14.09.2021 um 13:43 schrieb Ruediger Pluem <rpluem@apache.org>:
>>
>> When looking at this again while researching something I couldn't answer myself the questions below
>> and as event mpm and mod_http2 are sometimes pretty complex I thought I ask :-)
>>
>> On 7/3/19 3:46 PM, icing@apache.org wrote:
>>> Author: icing
>>> Date: Wed Jul 3 13:46:31 2019
>>> New Revision: 1862475
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1862475&view=rev
>>> Log:
>>> *) mod_http2/mpm_event: Fixes the behaviour when a HTTP/2 connection has nothing
>>> more to write with streams ongoing (flow control block). The timeout waiting
>>> for the client to send WINODW_UPDATE was incorrectly KeepAliveTimeout and not
>>> Timeout as it should be. Fixes PR 63534. [Yann Ylavic, Stefan Eissing]
>>>
>>>
>>> Modified:
>>> httpd/httpd/trunk/CHANGES
>>> httpd/httpd/trunk/modules/http2/h2_conn.c
>>> httpd/httpd/trunk/modules/http2/h2_version.h
>>> httpd/httpd/trunk/server/mpm/event/event.c
>>>
>>
>>> Modified: httpd/httpd/trunk/modules/http2/h2_conn.c
>>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_conn.c?rev=1862475&r1=1862474&r2=1862475&view=diff
>>> ==============================================================================
>>> --- httpd/httpd/trunk/modules/http2/h2_conn.c (original)
>>> +++ httpd/httpd/trunk/modules/http2/h2_conn.c Wed Jul 3 13:46:31 2019
>>> @@ -231,6 +231,13 @@ apr_status_t h2_conn_run(conn_rec *c)
>>> case H2_SESSION_ST_BUSY:
>>> case H2_SESSION_ST_WAIT:
>>> c->cs->state = CONN_STATE_WRITE_COMPLETION;
>>> + if (c->cs && (session->open_streams || !session->remote.emitted_count)) {
>>> + /* let the MPM know that we are not done and want
>>> + * the Timeout behaviour instead of a KeepAliveTimeout
>>> + * See PR 63534.
>>> + */
>>> + c->cs->sense = CONN_SENSE_WANT_READ;
>>
>> Can we get here with session->open_streams > 0 and all the open streams are only producing output and none of them is waiting for
>> something from the client?
>
> Yes, but only when the HTTP/2 flow control blocks any progress. New data has to arrive from the client before the server is able to send more. So, we are waiting for new data from the client in the form of window updates.

Thanks, but as you state in this case it wants to read data, not for a new request for getting output data sending for existing
requests started again. This sounds fine. I was worried that we could wait for input data here without expecting any.

>
>>> + }
>>> break;
>>> case H2_SESSION_ST_CLEANUP:
>>> case H2_SESSION_ST_DONE:
>>>
>>
>>> Modified: httpd/httpd/trunk/server/mpm/event/event.c
>>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/mpm/event/event.c?rev=1862475&r1=1862474&r2=1862475&view=diff
>>> ==============================================================================
>>> --- httpd/httpd/trunk/server/mpm/event/event.c (original)
>>> +++ httpd/httpd/trunk/server/mpm/event/event.c Wed Jul 3 13:46:31 2019
>>> @@ -1153,10 +1153,11 @@ read_request:
>>> else if (ap_filter_should_yield(c->output_filters)) {
>>> pending = OK;
>>> }
>>> - if (pending == OK) {
>>> + if (pending == OK || (pending == DECLINED &&
>>> + cs->pub.sense == CONN_SENSE_WANT_READ)) {
>>> /* Still in WRITE_COMPLETION_STATE:
>>> - * Set a write timeout for this connection, and let the
>>> - * event thread poll for writeability.
>>> + * Set a read/write timeout for this connection, and let the
>>> + * event thread poll for read/writeability.
>>> */
>>> cs->queue_timestamp = apr_time_now();
>>> notify_suspend(cs);
>>
>> Hm. Showing following code lines from trunk for my next question:
>>
>>
>> update_reqevents_from_sense(cs, -1);
>>
>> if cs->pub.sense == CONN_SENSE_WANT_READ we subscribe on to POLLIN only on the pfd
>>
>>
>> apr_thread_mutex_lock(timeout_mutex);
>> TO_QUEUE_APPEND(cs->sc->wc_q, cs);
>> rv = apr_pollset_add(event_pollset, &cs->pfd);
>>
>> If the read event triggers we will get back to the
>>
>> if (cs->pub.state == CONN_STATE_WRITE_COMPLETION) {
>> int pending = DECLINED;
>>
>> above. This means we try flush the output queue if it is not empty and if it is empty after this we would fall through to
>> set cs->pub.state = CONN_STATE_CHECK_REQUEST_LINE_READABLE; and then add the pfd to the pollset again and poll again. This should
>> trigger the read event again and we would process the input. But if I see this correctly we would need to poll twice for getting
>> the read data processed.
>> OTOH if we do not manage to clear the output queue above we would add the pfd to the pollset again but this time for writing and
>> only once all output has been processed we would take care of the reading.
>> Maybe we should take care of both?
>
> Hmm, but can it? In my understanding CONN_STATE_WRITE_COMPLETION is intended to flush the out buffers before putting in new work.

But a single call dies not need to flush all pending data from my understanding provided that no need to flush the data is found
like too much in memory data or too much EOR buckets which mean that we have too much finished requests in the output queue.
Hence CONN_STATE_WRITE_COMPLETION could happen multiple times only writing pieces from the data in the output queue.

>
> Since conn_rec's are limited to be processes in one thread-at-a-time only, POLLIN would call process_connection and that alone would block any more processing of the connection's output. Or?

Yes and no. If we take the CONN_SENSE_WANT_READ example from h2_conn above it would mean that no further processing of pending
output would happen until input is available since update_reqevents_from_sense(cs, -1) will cause that we only ask for read events
on the FD. This looks wrong to me in cases where we still have output data that we could write to the client, provided we get an
event for write on the socket. Hence my idea would be to ask for both and process them in write / read order provided that both
events get back. If only write came back only process write but keep asking for read. If only read came back process read. In all
cases only read comes back, we would not ask for any event on the FD any longer but just would process the input which should
cause the output processing to start again.


Regards

Rüdiger
Re: svn commit: r1862475 - in /httpd/httpd/trunk: CHANGES modules/http2/h2_conn.c modules/http2/h2_version.h server/mpm/event/event.c [ In reply to ]
> Am 20.09.2021 um 12:27 schrieb Ruediger Pluem <rpluem@apache.org>:
>
>
>
> On 9/20/21 11:17 AM, stefan@eissing.org wrote:
>>
>>
>>> Am 14.09.2021 um 13:43 schrieb Ruediger Pluem <rpluem@apache.org>:
>>>
>>> When looking at this again while researching something I couldn't answer myself the questions below
>>> and as event mpm and mod_http2 are sometimes pretty complex I thought I ask :-)
>>>
>>> On 7/3/19 3:46 PM, icing@apache.org wrote:
>>>> Author: icing
>>>> Date: Wed Jul 3 13:46:31 2019
>>>> New Revision: 1862475
>>>>
>>>> URL: http://svn.apache.org/viewvc?rev=1862475&view=rev
>>>> Log:
>>>> *) mod_http2/mpm_event: Fixes the behaviour when a HTTP/2 connection has nothing
>>>> more to write with streams ongoing (flow control block). The timeout waiting
>>>> for the client to send WINODW_UPDATE was incorrectly KeepAliveTimeout and not
>>>> Timeout as it should be. Fixes PR 63534. [Yann Ylavic, Stefan Eissing]
>>>>
>>>>
>>>> Modified:
>>>> httpd/httpd/trunk/CHANGES
>>>> httpd/httpd/trunk/modules/http2/h2_conn.c
>>>> httpd/httpd/trunk/modules/http2/h2_version.h
>>>> httpd/httpd/trunk/server/mpm/event/event.c
>>>>
>>>
>>>> Modified: httpd/httpd/trunk/modules/http2/h2_conn.c
>>>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_conn.c?rev=1862475&r1=1862474&r2=1862475&view=diff
>>>> ==============================================================================
>>>> --- httpd/httpd/trunk/modules/http2/h2_conn.c (original)
>>>> +++ httpd/httpd/trunk/modules/http2/h2_conn.c Wed Jul 3 13:46:31 2019
>>>> @@ -231,6 +231,13 @@ apr_status_t h2_conn_run(conn_rec *c)
>>>> case H2_SESSION_ST_BUSY:
>>>> case H2_SESSION_ST_WAIT:
>>>> c->cs->state = CONN_STATE_WRITE_COMPLETION;
>>>> + if (c->cs && (session->open_streams || !session->remote.emitted_count)) {
>>>> + /* let the MPM know that we are not done and want
>>>> + * the Timeout behaviour instead of a KeepAliveTimeout
>>>> + * See PR 63534.
>>>> + */
>>>> + c->cs->sense = CONN_SENSE_WANT_READ;
>>>
>>> Can we get here with session->open_streams > 0 and all the open streams are only producing output and none of them is waiting for
>>> something from the client?
>>
>> Yes, but only when the HTTP/2 flow control blocks any progress. New data has to arrive from the client before the server is able to send more. So, we are waiting for new data from the client in the form of window updates.
>
> Thanks, but as you state in this case it wants to read data, not for a new request for getting output data sending for existing
> requests started again. This sounds fine. I was worried that we could wait for input data here without expecting any.
>
>>
>>>> + }
>>>> break;
>>>> case H2_SESSION_ST_CLEANUP:
>>>> case H2_SESSION_ST_DONE:
>>>>
>>>
>>>> Modified: httpd/httpd/trunk/server/mpm/event/event.c
>>>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/mpm/event/event.c?rev=1862475&r1=1862474&r2=1862475&view=diff
>>>> ==============================================================================
>>>> --- httpd/httpd/trunk/server/mpm/event/event.c (original)
>>>> +++ httpd/httpd/trunk/server/mpm/event/event.c Wed Jul 3 13:46:31 2019
>>>> @@ -1153,10 +1153,11 @@ read_request:
>>>> else if (ap_filter_should_yield(c->output_filters)) {
>>>> pending = OK;
>>>> }
>>>> - if (pending == OK) {
>>>> + if (pending == OK || (pending == DECLINED &&
>>>> + cs->pub.sense == CONN_SENSE_WANT_READ)) {
>>>> /* Still in WRITE_COMPLETION_STATE:
>>>> - * Set a write timeout for this connection, and let the
>>>> - * event thread poll for writeability.
>>>> + * Set a read/write timeout for this connection, and let the
>>>> + * event thread poll for read/writeability.
>>>> */
>>>> cs->queue_timestamp = apr_time_now();
>>>> notify_suspend(cs);
>>>
>>> Hm. Showing following code lines from trunk for my next question:
>>>
>>>
>>> update_reqevents_from_sense(cs, -1);
>>>
>>> if cs->pub.sense == CONN_SENSE_WANT_READ we subscribe on to POLLIN only on the pfd
>>>
>>>
>>> apr_thread_mutex_lock(timeout_mutex);
>>> TO_QUEUE_APPEND(cs->sc->wc_q, cs);
>>> rv = apr_pollset_add(event_pollset, &cs->pfd);
>>>
>>> If the read event triggers we will get back to the
>>>
>>> if (cs->pub.state == CONN_STATE_WRITE_COMPLETION) {
>>> int pending = DECLINED;
>>>
>>> above. This means we try flush the output queue if it is not empty and if it is empty after this we would fall through to
>>> set cs->pub.state = CONN_STATE_CHECK_REQUEST_LINE_READABLE; and then add the pfd to the pollset again and poll again. This should
>>> trigger the read event again and we would process the input. But if I see this correctly we would need to poll twice for getting
>>> the read data processed.
>>> OTOH if we do not manage to clear the output queue above we would add the pfd to the pollset again but this time for writing and
>>> only once all output has been processed we would take care of the reading.
>>> Maybe we should take care of both?
>>
>> Hmm, but can it? In my understanding CONN_STATE_WRITE_COMPLETION is intended to flush the out buffers before putting in new work.
>
> But a single call dies not need to flush all pending data from my understanding provided that no need to flush the data is found
> like too much in memory data or too much EOR buckets which mean that we have too much finished requests in the output queue.
> Hence CONN_STATE_WRITE_COMPLETION could happen multiple times only writing pieces from the data in the output queue.
>
>>
>> Since conn_rec's are limited to be processes in one thread-at-a-time only, POLLIN would call process_connection and that alone would block any more processing of the connection's output. Or?
>
> Yes and no. If we take the CONN_SENSE_WANT_READ example from h2_conn above it would mean that no further processing of pending
> output would happen until input is available since update_reqevents_from_sense(cs, -1) will cause that we only ask for read events
> on the FD. This looks wrong to me in cases where we still have output data that we could write to the client, provided we get an
> event for write on the socket. Hence my idea would be to ask for both and process them in write / read order provided that both
> events get back. If only write came back only process write but keep asking for read. If only read came back process read. In all
> cases only read comes back, we would not ask for any event on the FD any longer but just would process the input which should
> cause the output processing to start again.
>

The "only POLLIN" case would explain behaviour I experienced in the past where output was sometimes not sent unless I really FLUSH it before a read. This change sounds good to me.

> Regards
>
> Rüdiger
Re: svn commit: r1862475 - in /httpd/httpd/trunk: CHANGES modules/http2/h2_conn.c modules/http2/h2_version.h server/mpm/event/event.c [ In reply to ]
On Mon, Sep 20, 2021 at 12:43 PM stefan@eissing.org <stefan@eissing.org> wrote:
>
> > Am 20.09.2021 um 12:27 schrieb Ruediger Pluem <rpluem@apache.org>:
> >
> > On 9/20/21 11:17 AM, stefan@eissing.org wrote:
> >>
> >>> Am 14.09.2021 um 13:43 schrieb Ruediger Pluem <rpluem@apache.org>:
> >>>
> >>>> --- httpd/httpd/trunk/server/mpm/event/event.c (original)
> >>>> +++ httpd/httpd/trunk/server/mpm/event/event.c Wed Jul 3 13:46:31 2019
> >>>> @@ -1153,10 +1153,11 @@ read_request:
> >>>> else if (ap_filter_should_yield(c->output_filters)) {
> >>>> pending = OK;
> >>>> }
> >>>> - if (pending == OK) {
> >>>> + if (pending == OK || (pending == DECLINED &&
> >>>> + cs->pub.sense == CONN_SENSE_WANT_READ)) {
> >>>> /* Still in WRITE_COMPLETION_STATE:
> >>>> - * Set a write timeout for this connection, and let the
> >>>> - * event thread poll for writeability.
> >>>> + * Set a read/write timeout for this connection, and let the
> >>>> + * event thread poll for read/writeability.
> >>>> */
> >>>> cs->queue_timestamp = apr_time_now();
> >>>> notify_suspend(cs);
> >>>
> >>> Hm. Showing following code lines from trunk for my next question:
> >>>
> >>> update_reqevents_from_sense(cs, -1);
> >>>
> >>> if cs->pub.sense == CONN_SENSE_WANT_READ we subscribe on to POLLIN only on the pfd
> >>>
> >>> apr_thread_mutex_lock(timeout_mutex);
> >>> TO_QUEUE_APPEND(cs->sc->wc_q, cs);
> >>> rv = apr_pollset_add(event_pollset, &cs->pfd);
> >>>
> >>> If the read event triggers we will get back to the
> >>>
> >>> if (cs->pub.state == CONN_STATE_WRITE_COMPLETION) {
> >>> int pending = DECLINED;
> >>>
> >>> above. This means we try flush the output queue if it is not empty and if it is empty after this we would fall through to
> >>> set cs->pub.state = CONN_STATE_CHECK_REQUEST_LINE_READABLE; and then add the pfd to the pollset again and poll again. This should
> >>> trigger the read event again and we would process the input. But if I see this correctly we would need to poll twice for getting
> >>> the read data processed.

For CONN_STATE_WRITE_COMPLETION + CONN_SENSE_WANT_READ we indeed reach
here and will POLLIN, then once readable we come back with
CONN_STATE_WRITE_COMPLETION + CONN_SENSE_DEFAULT so if pending != OK
we'll reach the below:

else if (ap_run_input_pending(c) == OK) {
cs->pub.state = CONN_STATE_READ_REQUEST_LINE;
goto read_request;
}

and thus ap_run_process_connection() directly.

> >>> OTOH if we do not manage to clear the output queue above we would add the pfd to the pollset again but this time for writing and
> >>> only once all output has been processed we would take care of the reading.
> >>> Maybe we should take care of both?
> >>
> >> Hmm, but can it? In my understanding CONN_STATE_WRITE_COMPLETION is intended to flush the out buffers before putting in new work.
> >
> > But a single call dies not need to flush all pending data from my understanding provided that no need to flush the data is found
> > like too much in memory data or too much EOR buckets which mean that we have too much finished requests in the output queue.
> > Hence CONN_STATE_WRITE_COMPLETION could happen multiple times only writing pieces from the data in the output queue.

If pending == OK while we asked for CONN_SENSE_WANT_READ we indeed
lose CONN_SENSE_WANT_READ when coming back here after POLLOUT.

What about this patch (attached)?
I tried to comment a bit the WRITE_COMPLETION_STATE too, because it's
not only about writing...


Cheers;
Yann.
Re: svn commit: r1862475 - in /httpd/httpd/trunk: CHANGES modules/http2/h2_conn.c modules/http2/h2_version.h server/mpm/event/event.c [ In reply to ]
On 9/20/21 1:31 PM, Yann Ylavic wrote:
> On Mon, Sep 20, 2021 at 12:43 PM stefan@eissing.org <stefan@eissing.org> wrote:
>>
>>> Am 20.09.2021 um 12:27 schrieb Ruediger Pluem <rpluem@apache.org>:
>>>
>>> On 9/20/21 11:17 AM, stefan@eissing.org wrote:
>>>>
>>>>> Am 14.09.2021 um 13:43 schrieb Ruediger Pluem <rpluem@apache.org>:
>>>>>
>>>>>> --- httpd/httpd/trunk/server/mpm/event/event.c (original)
>>>>>> +++ httpd/httpd/trunk/server/mpm/event/event.c Wed Jul 3 13:46:31 2019
>>>>>> @@ -1153,10 +1153,11 @@ read_request:
>>>>>> else if (ap_filter_should_yield(c->output_filters)) {
>>>>>> pending = OK;
>>>>>> }
>>>>>> - if (pending == OK) {
>>>>>> + if (pending == OK || (pending == DECLINED &&
>>>>>> + cs->pub.sense == CONN_SENSE_WANT_READ)) {
>>>>>> /* Still in WRITE_COMPLETION_STATE:
>>>>>> - * Set a write timeout for this connection, and let the
>>>>>> - * event thread poll for writeability.
>>>>>> + * Set a read/write timeout for this connection, and let the
>>>>>> + * event thread poll for read/writeability.
>>>>>> */
>>>>>> cs->queue_timestamp = apr_time_now();
>>>>>> notify_suspend(cs);
>>>>>
>>>>> Hm. Showing following code lines from trunk for my next question:
>>>>>
>>>>> update_reqevents_from_sense(cs, -1);
>>>>>
>>>>> if cs->pub.sense == CONN_SENSE_WANT_READ we subscribe on to POLLIN only on the pfd
>>>>>
>>>>> apr_thread_mutex_lock(timeout_mutex);
>>>>> TO_QUEUE_APPEND(cs->sc->wc_q, cs);
>>>>> rv = apr_pollset_add(event_pollset, &cs->pfd);
>>>>>
>>>>> If the read event triggers we will get back to the
>>>>>
>>>>> if (cs->pub.state == CONN_STATE_WRITE_COMPLETION) {
>>>>> int pending = DECLINED;
>>>>>
>>>>> above. This means we try flush the output queue if it is not empty and if it is empty after this we would fall through to
>>>>> set cs->pub.state = CONN_STATE_CHECK_REQUEST_LINE_READABLE; and then add the pfd to the pollset again and poll again. This should
>>>>> trigger the read event again and we would process the input. But if I see this correctly we would need to poll twice for getting
>>>>> the read data processed.
>
> For CONN_STATE_WRITE_COMPLETION + CONN_SENSE_WANT_READ we indeed reach
> here and will POLLIN, then once readable we come back with
> CONN_STATE_WRITE_COMPLETION + CONN_SENSE_DEFAULT so if pending != OK
> we'll reach the below:
>
> else if (ap_run_input_pending(c) == OK) {
> cs->pub.state = CONN_STATE_READ_REQUEST_LINE;
> goto read_request;
> }
>
> and thus ap_run_process_connection() directly.

Thanks for the pointer. But I guess the similar

else if (c->data_in_input_filters) {
cs->pub.state = CONN_STATE_READ_REQUEST_LINE;
goto read_request;

in 2.4.x does not give this to us as I think that c->data_in_input_filters is not updated until we get here. Hence we probably
need to fix this differently here.

>
>>>>> OTOH if we do not manage to clear the output queue above we would add the pfd to the pollset again but this time for writing and
>>>>> only once all output has been processed we would take care of the reading.
>>>>> Maybe we should take care of both?
>>>>
>>>> Hmm, but can it? In my understanding CONN_STATE_WRITE_COMPLETION is intended to flush the out buffers before putting in new work.
>>>
>>> But a single call dies not need to flush all pending data from my understanding provided that no need to flush the data is found
>>> like too much in memory data or too much EOR buckets which mean that we have too much finished requests in the output queue.
>>> Hence CONN_STATE_WRITE_COMPLETION could happen multiple times only writing pieces from the data in the output queue.
>
> If pending == OK while we asked for CONN_SENSE_WANT_READ we indeed
> lose CONN_SENSE_WANT_READ when coming back here after POLLOUT.
>
> What about this patch (attached)?
> I tried to comment a bit the WRITE_COMPLETION_STATE too, because it's
> not only about writing...
>

> Index: server/mpm/event/event.c
> ===================================================================
> --- server/mpm/event/event.c (revision 1893073)
> +++ server/mpm/event/event.c (working copy)
> @@ -999,7 +1001,7 @@ static void process_socket(apr_thread_t *thd, apr_
> {
> conn_rec *c;
> long conn_id = ID_FROM_CHILD_THREAD(my_child_num, my_thread_num);
> - int clogging = 0, from_wc_q = 0;
> + int clogging = 0;
> apr_status_t rv;
> int rc = OK;
>
> @@ -1101,9 +1103,6 @@ read_request:
> rc = OK;
> }
> }
> - else if (cs->pub.state == CONN_STATE_WRITE_COMPLETION) {
> - from_wc_q = 1;
> - }
> }
> /*
> * The process_connection hooks above should set the connection state
> @@ -1146,20 +1145,28 @@ read_request:
> cs->pub.state = CONN_STATE_LINGER;
> }
>
> + /* CONN_STATE_WRITE_COMPLETION is a misnomer, this is actually the
> + * user driver polling state which can be POLLOUT but also POLLIN
> + * depending on cs->pub.sense. But even for CONN_SENSE_WANT_READ
> + * we want the outgoing pipe to be empty before POLLIN, so there

Why do we need to empty the outgoing pipe first? It will automatically go into blocking once it thinks that it is needed. If not I
think that we can start processing input in parallel. If this produces more output that cannot be queued we get into blocking
anyway. The only drawback of starting input processing again could be that if the request we start processing is running for a
long time the output will not be flushed further until new output was produced. And I guess working on the same connection
in two different threads would not be possible as the connection structure is not thread safe.

> + * will always be write completion first.
> + */
> if (cs->pub.state == CONN_STATE_WRITE_COMPLETION) {
> - int pending = DECLINED;
> + int pending, want_read = 0;
>
> - ap_update_child_status(cs->sbh, SERVER_BUSY_WRITE, NULL);
> -
> - if (from_wc_q) {
> - from_wc_q = 0; /* one shot */
> - pending = ap_run_output_pending(c);
> + if (cs->pub.sense == CONN_SENSE_DEFAULT) {
> + /* If the user did not ask for READ or WRITE explicitely then
> + * we are in normal post request processing, i.e. busy write
> + * in the scoreboard.
> + */
> + ap_update_child_status(cs->sbh, SERVER_BUSY_WRITE, NULL);
> }
> - else if (ap_filter_should_yield(c->output_filters)) {

Why no longer the ap_filter_should_yield(c->output_filters) approach when a proccesing just before left us in write completion?

> - pending = OK;
> + else if (cs->pub.sense == CONN_SENSE_WANT_READ) {
> + want_read = 1;
> }
> - if (pending == OK || (pending == DECLINED &&
> - cs->pub.sense == CONN_SENSE_WANT_READ)) {
> +
> + pending = ap_run_output_pending(c);
> + if (pending == OK || (pending == DECLINED && want_read)) {
> /* Still in WRITE_COMPLETION_STATE:
> * Set a read/write timeout for this connection, and let the
> * event thread poll for read/writeability.


Regards

Rüdiger
Re: svn commit: r1862475 - in /httpd/httpd/trunk: CHANGES modules/http2/h2_conn.c modules/http2/h2_version.h server/mpm/event/event.c [ In reply to ]
On Tue, Sep 21, 2021 at 12:25 PM Ruediger Pluem <rpluem@apache.org> wrote:
>
> On 9/20/21 1:31 PM, Yann Ylavic wrote:
> >
> > For CONN_STATE_WRITE_COMPLETION + CONN_SENSE_WANT_READ we indeed reach
> > here and will POLLIN, then once readable we come back with
> > CONN_STATE_WRITE_COMPLETION + CONN_SENSE_DEFAULT so if pending != OK
> > we'll reach the below:
> >
> > else if (ap_run_input_pending(c) == OK) {
> > cs->pub.state = CONN_STATE_READ_REQUEST_LINE;
> > goto read_request;
> > }
> >
> > and thus ap_run_process_connection() directly.
>
> Thanks for the pointer. But I guess the similar
>
> else if (c->data_in_input_filters) {
> cs->pub.state = CONN_STATE_READ_REQUEST_LINE;
> goto read_request;
>
> in 2.4.x does not give this to us as I think that c->data_in_input_filters is not updated until we get here. Hence we probably
> need to fix this differently here.

Yes, possibly a speculative read of one byte on c->input_filters..

> >
> > What about this patch (attached)?
> > I tried to comment a bit the WRITE_COMPLETION_STATE too, because it's
> > not only about writing...
> >
> > + /* CONN_STATE_WRITE_COMPLETION is a misnomer, this is actually the
> > + * user driver polling state which can be POLLOUT but also POLLIN
> > + * depending on cs->pub.sense. But even for CONN_SENSE_WANT_READ
> > + * we want the outgoing pipe to be empty before POLLIN, so there
>
> Why do we need to empty the outgoing pipe first? It will automatically go into blocking once it thinks that it is needed.

If we don't flush what we have retained we might be in a situation
where the remote is waiting for these data before sending us
something, and we'd deadlock.

> If not I
> think that we can start processing input in parallel. If this produces more output that cannot be queued we get into blocking
> anyway. The only drawback of starting input processing again could be that if the request we start processing is running for a
> long time the output will not be flushed further until new output was produced. And I guess working on the same connection
> in two different threads would not be possible as the connection structure is not thread safe.

I don't think we should ever have the same connection handled by
multiple threads, that's a can of worms where we'd have to synchronize
at some point and determine which packet arrived first. Packets need
to be handled in sequence, when a connection is handled by a worker
thread it should never be in the MPM's pollset at the same time (i.e.
possibly scheduled by/to another thread).

> >
> > if (cs->pub.state == CONN_STATE_WRITE_COMPLETION) {
> > - int pending = DECLINED;
> > + int pending, want_read = 0;
> >
> > - ap_update_child_status(cs->sbh, SERVER_BUSY_WRITE, NULL);
> > -
> > - if (from_wc_q) {
> > - from_wc_q = 0; /* one shot */
> > - pending = ap_run_output_pending(c);
> > + if (cs->pub.sense == CONN_SENSE_DEFAULT) {
> > + /* If the user did not ask for READ or WRITE explicitely then
> > + * we are in normal post request processing, i.e. busy write
> > + * in the scoreboard.
> > + */
> > + ap_update_child_status(cs->sbh, SERVER_BUSY_WRITE, NULL);
> > }
> > - else if (ap_filter_should_yield(c->output_filters)) {
>
> Why no longer the ap_filter_should_yield(c->output_filters) approach when a proccesing just before left us in write completion?

I just wanted to "show" in the patch that we'd better call
ap_run_output_pending() when CONN_SENSE_WANT_READ to avoid the POLLOUT
if we can flush just now. ap_filter_should_yield() is an optimization
which avoids running the output filters if we already ran them or
called ap_run_output_pending() just before, but it's not necessarily
the case when CONN_SENSE_WANT_READ is asked (I suppose).
Anyway, I think we should ap_run_output_pending() in more cases then
the current "from_wc_q" (so I removed this logic), the right test is
possibly something along "from_wc_q || want_read" or something, or we
can just let ap_filter_should_yield() as is and do the POLLOUT even if
writing is non-blocking already..


Regards;
Yann.
Re: svn commit: r1862475 - in /httpd/httpd/trunk: CHANGES modules/http2/h2_conn.c modules/http2/h2_version.h server/mpm/event/event.c [ In reply to ]
On 9/21/21 2:10 PM, Yann Ylavic wrote:
> On Tue, Sep 21, 2021 at 12:25 PM Ruediger Pluem <rpluem@apache.org> wrote:
>>
>> On 9/20/21 1:31 PM, Yann Ylavic wrote:
>>>
>>> For CONN_STATE_WRITE_COMPLETION + CONN_SENSE_WANT_READ we indeed reach
>>> here and will POLLIN, then once readable we come back with
>>> CONN_STATE_WRITE_COMPLETION + CONN_SENSE_DEFAULT so if pending != OK
>>> we'll reach the below:
>>>
>>> else if (ap_run_input_pending(c) == OK) {
>>> cs->pub.state = CONN_STATE_READ_REQUEST_LINE;
>>> goto read_request;
>>> }
>>>
>>> and thus ap_run_process_connection() directly.

Having a look at this again this looks like to me that it only works if there is still stuff in the input filters and not if they
are empty and on the socket there is something to read. How about the below in addition:

Index: server/mpm/event/event.c
===================================================================
--- server/mpm/event/event.c (revision 1893352)
+++ server/mpm/event/event.c (working copy)
@@ -2042,7 +2041,17 @@
switch (cs->pub.state) {
case CONN_STATE_WRITE_COMPLETION:
remove_from_q = cs->sc->wc_q;
- blocking = 1;
+ /* We are in state CONN_STATE_WRITE_COMPLETION, but we asked
+ * for read and got only read back. In this case start
+ * reading a new request.
+ */
+ if ((out_pfd->reqevents & APR_POLLIN)
+ && (out_pfd->rtnevents == APR_POLLIN)) {
+ cs->pub.state = CONN_STATE_READ_REQUEST_LINE;
+ }
+ else {
+ blocking = 1;
+ }
break;

case CONN_STATE_CHECK_REQUEST_LINE_READABLE:


>>
>> Thanks for the pointer. But I guess the similar
>>
>> else if (c->data_in_input_filters) {
>> cs->pub.state = CONN_STATE_READ_REQUEST_LINE;
>> goto read_request;
>>
>> in 2.4.x does not give this to us as I think that c->data_in_input_filters is not updated until we get here. Hence we probably
>> need to fix this differently here.
>
> Yes, possibly a speculative read of one byte on c->input_filters..
>
>>>
>>> What about this patch (attached)?
>>> I tried to comment a bit the WRITE_COMPLETION_STATE too, because it's
>>> not only about writing...
>>>
>>> + /* CONN_STATE_WRITE_COMPLETION is a misnomer, this is actually the
>>> + * user driver polling state which can be POLLOUT but also POLLIN
>>> + * depending on cs->pub.sense. But even for CONN_SENSE_WANT_READ
>>> + * we want the outgoing pipe to be empty before POLLIN, so there
>>
>> Why do we need to empty the outgoing pipe first? It will automatically go into blocking once it thinks that it is needed.
>
> If we don't flush what we have retained we might be in a situation
> where the remote is waiting for these data before sending us
> something, and we'd deadlock.

But if we asked for reading we know that there is something to read and could process it so I don't see the possible deadlock
here, but the other point I mentioned remains: If this request takes a long time before it sends further output the remaining
output stays on our side. OTOH in certain pipelining scenarios we don't do this and only force a blocking flush if the pending
requests in output are above FlushMaxPipelined.

>
>> If not I
>> think that we can start processing input in parallel. If this produces more output that cannot be queued we get into blocking
>> anyway. The only drawback of starting input processing again could be that if the request we start processing is running for a
>> long time the output will not be flushed further until new output was produced. And I guess working on the same connection
>> in two different threads would not be possible as the connection structure is not thread safe.
>
> I don't think we should ever have the same connection handled by
> multiple threads, that's a can of worms where we'd have to synchronize
> at some point and determine which packet arrived first. Packets need
> to be handled in sequence, when a connection is handled by a worker
> thread it should never be in the MPM's pollset at the same time (i.e.
> possibly scheduled by/to another thread).

This is what I thought as well: I would be nice, but it is a can of worms.

>
>>>
>>> if (cs->pub.state == CONN_STATE_WRITE_COMPLETION) {
>>> - int pending = DECLINED;
>>> + int pending, want_read = 0;
>>>
>>> - ap_update_child_status(cs->sbh, SERVER_BUSY_WRITE, NULL);
>>> -
>>> - if (from_wc_q) {
>>> - from_wc_q = 0; /* one shot */
>>> - pending = ap_run_output_pending(c);
>>> + if (cs->pub.sense == CONN_SENSE_DEFAULT) {
>>> + /* If the user did not ask for READ or WRITE explicitely then
>>> + * we are in normal post request processing, i.e. busy write
>>> + * in the scoreboard.
>>> + */
>>> + ap_update_child_status(cs->sbh, SERVER_BUSY_WRITE, NULL);
>>> }
>>> - else if (ap_filter_should_yield(c->output_filters)) {
>>
>> Why no longer the ap_filter_should_yield(c->output_filters) approach when a proccesing just before left us in write completion?
>
> I just wanted to "show" in the patch that we'd better call
> ap_run_output_pending() when CONN_SENSE_WANT_READ to avoid the POLLOUT
> if we can flush just now. ap_filter_should_yield() is an optimization
> which avoids running the output filters if we already ran them or
> called ap_run_output_pending() just before, but it's not necessarily
> the case when CONN_SENSE_WANT_READ is asked (I suppose).
> Anyway, I think we should ap_run_output_pending() in more cases then
> the current "from_wc_q" (so I removed this logic), the right test is
> possibly something along "from_wc_q || want_read" or something, or we
> can just let ap_filter_should_yield() as is and do the POLLOUT even if
> writing is non-blocking already..

Thanks for explaining. This makes sense.


Regards

Rüdiger