On Mon, Jan 24, 2022 at 5:19 PM Graham Leggett <minf...@sharp.fm> wrote:
>
> On 24 Jan 2022, at 13:56, Yann Ylavic <ylavic....@gmail.com> wrote:
>
> >> --- httpd/httpd/trunk/server/mpm/event/event.c (original)
> >> +++ httpd/httpd/trunk/server/mpm/event/event.c Fri Jan 21 00:09:24 2022
> >> @@ -1142,6 +1145,7 @@ read_request:
> >> */
> >> if (rc != OK || (cs->pub.state >= CONN_STATE_NUM)
> >> || (cs->pub.state < CONN_STATE_LINGER
> >> + && cs->pub.state != CONN_STATE_READ_REQUEST_LINE
> >> && cs->pub.state != CONN_STATE_WRITE_COMPLETION
> >> && cs->pub.state !=
> >> CONN_STATE_CHECK_REQUEST_LINE_READABLE
> >> && cs->pub.state != CONN_STATE_SUSPENDED)) {
> >
> > The issue is that we've never asked process_connection hooks that
> > don't care about async and co to change cs->pub.state when they are
> > done, and for instance mod_echo's process_echo_connection() will
> > consume the whole connection but leave CONN_STATE_READ_REQUEST_LINE as
> > is, and here we'll no longer turn it to CONN_STATE_LINGER to close the
> > connection as before hence loop indefinitely between mod_echo and
> > mpm_event (eating a CPU) for a connection that is EOF already.
>
> True - fixed this in r1897423.
>
> Instead of assuming that all async MPM’s can handle AGAIN, added a dedicated
> MPM check for it. This mean no surprises for existing code.
Why? Really I think you are complicating this. Every async MPM should
be able to handle EAGAIN, i.e. AP_MPMQ_IS_ASYNC == AP_MPM_CAN_AGAIN.
So far (as you said in your other response) there was only one way to
go async: register a callback onto the MPM with
ap_mpm_register_{timed,poll}_callback() and return SUSPENDED to httpd
for process_connection to set CONN_STATE_SUSPENDED before returning
(with OK!) to the MPM. This tells the MPM that the connection has been
queued by other means (nothing to do on its side).
Yet the connection is suspended (be it to the MPM or not), and the MPM
will notify_suspend() before ignoring it for now (meaning that if the
module suspended it to something else than the MPM it'd need to
ap_run_suspend_connection() by itself and notify_resume() too when
it's rescheduled).
A callback was registered though so whenever the event fires the MPM
will unregister it, notify_resume() and call it.
If the callback wants to be scheduled by the MPM again after running
it needs to call ap_mpm_register_*_callback() again and finally
ap_mpm_resume_suspended() (i.e. suspend the connection again and call
me back etc, or set CONN_STATE_LINGER to let the MPM kill it).
In dialup_handler() we are not in a position where we can simply
return SUSPENDED to the MPM (from process_connection) and expect that
it calls process_connection() again when the timer/IO fires and we can
come back to dialup_handler() where it left without anything else
interacting until then, that why there is a callback mechanism.
Now we want to be able to suspend in early connection processing,
before anything happened on the connection or any request was
created/handled (i.e. AP_MODE_INIT) and know that the MPM can simply
call process_connection() again when it's ready.
I argue that returning SUSPENDED from a process_connection hook that
knows what it's doing is the simpler thing to do, without breaking any
semantics..
Regards;
Yann.
