On Fri, Jan 21, 2022 at 1:09 AM <minf...@apache.org> wrote:
>
> Author: minfrin
> Date: Fri Jan 21 00:09:24 2022
> New Revision: 1897281
>
> URL: http://svn.apache.org/viewvc?rev=1897281&view=rev
> Log:
> event: Add support for non blocking behaviour in the
> CONN_STATE_READ_REQUEST_LINE phase, in addition to the existing
> CONN_STATE_WRITE_COMPLETION phase. Update mod_ssl to perform non blocking
> TLS handshakes.
It looks like reusing CONN_STATE_READ_REQUEST_LINE for that is causing
issues, see below.
> --- httpd/httpd/trunk/modules/ssl/mod_ssl.c (original)
> +++ httpd/httpd/trunk/modules/ssl/mod_ssl.c Fri Jan 21 00:09:24 2022
> @@ -691,6 +692,8 @@ static int ssl_hook_process_connection(c
> {
> SSLConnRec *sslconn = myConnConfig(c);
>
> + int status = DECLINED;
> +
> if (sslconn && !sslconn->disabled) {
> /* On an active SSL connection, let the input filters initialize
> * themselves which triggers the handshake, which again triggers
> @@ -698,13 +701,48 @@ static int ssl_hook_process_connection(c
> */
> apr_bucket_brigade* temp;
>
> + int async_mpm = 0;
> +
> temp = apr_brigade_create(c->pool, c->bucket_alloc);
> - ap_get_brigade(c->input_filters, temp,
> - AP_MODE_INIT, APR_BLOCK_READ, 0);
> +
> + if (ap_mpm_query(AP_MPMQ_IS_ASYNC, &async_mpm) != APR_SUCCESS) {
> + async_mpm = 0;
> + }
> +
> + if (async_mpm) {
> +
> + /* Take advantage of an async MPM. If we see an EAGAIN,
> + * loop round and don't block.
> + */
> + apr_status_t rv;
> +
> + rv = ap_get_brigade(c->input_filters, temp,
> + AP_MODE_INIT, APR_NONBLOCK_READ, 0);
> +
> + if (rv == APR_SUCCESS) {
> + /* great news, lets continue */
> + status = DECLINED;
> + }
> + else if (rv == APR_EAGAIN) {
> + /* we've been asked to come around again, don't block */
> + status = OK;
Maybe we could return SUSPENDED here to tell the MPM that it should
call run_process_connection() again after poll()ing.
> + }
> + else {
> + /* we failed, give up */
> + status = DONE;
> +
> + c->aborted = 1;
> + }
> + }
> + else {
> + ap_get_brigade(c->input_filters, temp,
> + AP_MODE_INIT, APR_BLOCK_READ, 0);
> + }
> +
> apr_brigade_destroy(temp);
> }
> -
> - return DECLINED;
> +
> + return status;
> }
> --- 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.
That's why I'm proposing above that ssl_hook_process_connection()
returns SUSPENDED and that we handle it as keep calling
run_process_connection() after poll()ing here in the MPM. Or have a
new CONN_STATE_WAIT_IO or something set by the hooks, though since we
have SUSPENDED already it looks unnecessary.
> @@ -1153,6 +1157,41 @@ read_request:
> cs->pub.state = CONN_STATE_LINGER;
> }
Regards;
Yann.
