If I understand your gist correctly, this would allow HTTP/2 processing to 
return to the main (async) event loop more often. Which would be great.

In the case of HTTP/2, it would be even more cool, to trigger the 
(re-)processing of an AGAIN connection from another thread. The use
case is: H2 main look started request, awaits response HEADERS/DATA *or* 
incoming data from client.

 now: timed wait on a condition with read checks on the main connection at 
dynamic intervals
then: return AGAIN (READ mode) to event look, new HEADERS/DATA from request 
triggers re-process_connection.

-Stefan



> Am 18.02.2018 um 00:11 schrieb Graham Leggett <minf...@sharp.fm>:
> 
> Hi all,
> 
> As an extension to the idea of filters being async and being able to yield 
> and break up their work into small chunks, I am keen to extend that idea to 
> selected hooks.
> 
> The patch below is a proof of concept, showing what it might look like if the 
> pre_connection and process_connection hooks were able to return the code 
> AGAIN. This means “please call me again”.
> 
> In implementing this, we would start at the outermost hook, and then work 
> inwards. We would also need to change these void functions to return ints, so 
> that AGAIN could bubble upwards. Eventually the handler hook will be able to 
> return AGAIN.
> 
> Any module that doesn’t yield just behaves as modules do now.
> 
> Does this make sense?
> 
> Regards,
> Graham
> --
> 
> Index: include/http_connection.h
> ===================================================================
> --- include/http_connection.h (revision 1824594)
> +++ include/http_connection.h (working copy)
> @@ -105,11 +105,14 @@
>  * This hook gives protocol modules an opportunity to set everything up
>  * before calling the protocol handler.  All pre-connection hooks are
>  * run until one returns something other than ok or decline
> + *
> + * The pre connection hook may trigger a break in processing and ask to
> + * be called again by returning AGAIN.
>  * @param c The connection on which the request has been received.
>  * @param csd The mechanism on which this connection is to be read.
>  *            Most times this will be a socket, but it is up to the module
>  *            that accepts the request to determine the exact type.
> - * @return OK or DECLINED
> + * @return OK, DECLINED or AGAIN
>  */
> AP_DECLARE_HOOK(int,pre_connection,(conn_rec *c, void *csd))
> 
> @@ -118,8 +121,11 @@
>  * established, the protocol module must read and serve the request.  This
>  * function does that for each protocol module.  The first protocol module
>  * to handle the request is the last module run.
> + *
> + * Protocol modules may trigger a break in processing and ask to be called
> + * again by returning AGAIN.
>  * @param c The connection on which the request has been received.
> - * @return OK or DECLINED
> + * @return OK, DECLINED or AGAIN
>  */
> AP_DECLARE_HOOK(int,process_connection,(conn_rec *c))
> 
> Index: include/httpd.h
> ===================================================================
> --- include/httpd.h   (revision 1824594)
> +++ include/httpd.h   (working copy)
> @@ -460,6 +460,8 @@
>                                  */
> #define SUSPENDED -3 /**< Module will handle the remainder of the request.
>                       * The core will never invoke the request again, */
> +#define AGAIN -4                /**< Module wants to be called again.
> +                                  */
> 
> /** Returned by the bottom-most filter if no data was written.
>  *  @see ap_pass_brigade(). */
> Index: server/connection.c
> ===================================================================
> --- server/connection.c       (revision 1824594)
> +++ server/connection.c       (working copy)
> @@ -29,6 +29,7 @@
> #include "scoreboard.h"
> #include "http_log.h"
> #include "util_filter.h"
> +#include "core.h"
> 
> APR_HOOK_STRUCT(
>             APR_HOOK_LINK(create_connection)
> @@ -207,15 +208,38 @@
> 
> AP_CORE_DECLARE(void) ap_process_connection(conn_rec *c, void *csd)
> {
> +    conn_config_t *conn_config = ap_get_core_module_config(c->conn_config);
>     int rc;
> +
> +    switch (AP_CORE_DEFAULT(conn_config, milestone, CONN_MILESTONE_START)) {
> +    case CONN_MILESTONE_START:
> +
>     ap_update_vhost_given_ip(c);
> 
> +    case CONN_MILESTONE_PRE_CONNECTION:
> +    conn_config->milestone = CONN_MILESTONE_START;
>     rc = ap_run_pre_connection(c, csd);
> +    if (rc == AGAIN) {
> +        conn_config->milestone = CONN_MILESTONE_PRE_CONNECTION;
> +        return;
> +    }
> +
>     if (rc != OK && rc != DONE) {
>         c->aborted = 1;
>     }
> 
>     if (!c->aborted) {
> -        ap_run_process_connection(c);
> +
> +        case CONN_MILESTONE_PROCESS_CONNECTION:
> +        conn_config->milestone = CONN_MILESTONE_START;
> +        rc = ap_run_process_connection(c);
> +        if (rc == AGAIN) {
> +            conn_config->milestone = CONN_MILESTONE_PROCESS_CONNECTION;
> +            return;
> +        }
> +
>     }
> +
> +    }
> }
> +
> Index: server/core.h
> ===================================================================
> --- server/core.h     (revision 1824635)
> +++ server/core.h     (working copy)
> @@ -25,6 +25,13 @@
> #ifndef CORE_H
> #define CORE_H
> 
> +/** The connection milestones in the core */
> +typedef enum  {
> +    CONN_MILESTONE_START,
> +    CONN_MILESTONE_PRE_CONNECTION,
> +    CONN_MILESTONE_PROCESS_CONNECTION
> +} conn_milestone_e;
> +
> /**
>  * @brief A structure to contain connection state information
>  */
> @@ -31,6 +38,8 @@
> typedef struct conn_config_t {
>     /** Socket belonging to the connection */
>     apr_socket_t *socket;
> +    /** Connection milestones */
> +    conn_milestone_e milestone;
> } conn_config_t;
> 
> 
> Index: server/mpm/event/event.c
> ===================================================================
> --- server/mpm/event/event.c  (revision 1824594)
> +++ server/mpm/event/event.c  (working copy)
> @@ -219,6 +219,14 @@
>  */
> static apr_pollset_t *event_pollset;
> 
> +typedef enum event_milestone_e event_milestone_e;
> +
> +enum event_milestone_e {
> +    EVENT_MILESTONE_START = 0,
> +    EVENT_MILESTONE_PRE_CONNECTION,
> +    EVENT_MILESTONE_PROCESS_CONNECTION
> +};
> +
> typedef struct event_conn_state_t event_conn_state_t;
> 
> /*
> @@ -245,6 +253,8 @@
>      * hooks)
>      */
>     int suspended;
> +    /** current milestone */
> +    event_milestone_e milestone;
>     /** memory pool to allocate from */
>     apr_pool_t *p;
>     /** bucket allocator */
> @@ -996,6 +1006,9 @@
>     apr_status_t rv;
>     int rc = OK;
> 
> +    switch (AP_CORE_DEFAULT(cs, milestone, EVENT_MILESTONE_START)) {
> +    case EVENT_MILESTONE_START:
> +
>     if (cs == NULL) {           /* This is a new connection */
>         listener_poll_type *pt = apr_pcalloc(p, sizeof(*pt));
>         cs = apr_pcalloc(p, sizeof(event_conn_state_t));
> @@ -1028,7 +1041,14 @@
> 
>         ap_update_vhost_given_ip(c);
> 
> +        case EVENT_MILESTONE_PRE_CONNECTION:
> +        cs->milestone = EVENT_MILESTONE_START;
>         rc = ap_run_pre_connection(c, sock);
> +        if (rc == AGAIN) {
> +            cs->milestone = EVENT_MILESTONE_PRE_CONNECTION;
> +            return;
> +        }
> +
>         if (rc != OK && rc != DONE) {
>             ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, c, APLOGNO(00469)
>                           "process_socket: connection aborted");
> @@ -1083,7 +1103,15 @@
>             if (clogging) {
>                 apr_atomic_inc32(&clogged_count);
>             }
> +
> +            case EVENT_MILESTONE_PROCESS_CONNECTION:
> +            cs->milestone = EVENT_MILESTONE_START;
>             rc = ap_run_process_connection(c);
> +            if (rc == AGAIN) {
> +                cs->milestone = EVENT_MILESTONE_PROCESS_CONNECTION;
> +                return;
> +            }
> +
>             if (clogging) {
>                 apr_atomic_dec32(&clogged_count);
>             }
> @@ -1095,6 +1123,7 @@
>             }
>         }
>     }
> +
>     /*
>      * The process_connection hooks above should set the connection state
>      * appropriately upon return, for event MPM to either:
> @@ -1245,6 +1274,8 @@
>                      cs->pub.state == CONN_STATE_LINGER_SHORT)) {
>         process_lingering_close(cs);
>     }
> +
> +    }
> }
> 
> /* Put a SUSPENDED connection back into a queue. */
> 
> 
> Regards,
> Graham
> —
> 

Reply via email to