On 9/20/21 11:17 AM, ste...@eissing.org wrote:
>
>
>> Am 14.09.2021 um 13:43 schrieb Ruediger Pluem <rpl...@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, ic...@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
