> Am 20.09.2021 um 12:27 schrieb Ruediger Pluem <rpl...@apache.org>:
>
>
>
> 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.
>
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
