On 9/20/21 1:31 PM, Yann Ylavic wrote:
> On Mon, Sep 20, 2021 at 12:43 PM ste...@eissing.org <ste...@eissing.org>
> wrote:
>>
>>> 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>:
>>>>>
>>>>>> --- 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
