Commit a0f51852 authored by Stefan Eissing's avatar Stefan Eissing
Browse files

On the trunk: 

  *) mod_http2: moving session cleanup to pre_close hook to avoid races with
     modules already shut down and slave connections still operating.



git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1786512 13f79535-47bb-0310-9956-ffa450edef68
parent c061f3cc

CHANGES

+4 −0
Original line number Diff line number Diff line 
                                                         -*- coding: utf-8 -*-

Changes with Apache 2.5.0



  *) mod_http2: moving session cleanup to pre_close hook to avoid races with

     modules already shut down and slave connections still operating.

     [Stefan Eissing]



  *) Add <IfDirective> and <IfSection> directives.  [Joe Orton]



  *) mod_http2: stream timeouts now change to vhost values once the request

modules/http2/h2_conn.c

+26 −12
Original line number Diff line number Diff line
@@ -183,6 +183,7 @@ static module *h2_conn_mpm_module(void) 
apr_status_t h2_conn_setup(h2_ctx *ctx, conn_rec *c, request_rec *r)

{

    h2_session *session;

    apr_status_t status;

    

    if (!workers) {

        ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(02911)
@@ -191,15 +192,16 @@ apr_status_t h2_conn_setup(h2_ctx *ctx, conn_rec *c, request_rec *r) 
    }

    

    if (r) {

        session = h2_session_rcreate(r, ctx, workers);

        status = h2_session_rcreate(&session, r, ctx, workers);

    }

    else {

        session = h2_session_create(c, ctx, workers);

        status = h2_session_create(&session, c, ctx, workers);

    }



    if (status == APR_SUCCESS) {

        h2_ctx_session_set(ctx, session);

    

    return APR_SUCCESS;

    }

    return status;

}



apr_status_t h2_conn_run(struct h2_ctx *ctx, conn_rec *c)
@@ -235,7 +237,20 @@ apr_status_t h2_conn_run(struct h2_ctx *ctx, conn_rec *c) 
             && mpm_state != AP_MPMQ_STOPPING);

    

    if (c->cs) {

        switch (session->state) {

            case H2_SESSION_ST_INIT:

            case H2_SESSION_ST_CLEANUP:

            case H2_SESSION_ST_DONE:

            case H2_SESSION_ST_IDLE:

                c->cs->state = CONN_STATE_WRITE_COMPLETION;

                break;

            case H2_SESSION_ST_BUSY:

            case H2_SESSION_ST_WAIT:

            default:

                c->cs->state = CONN_STATE_HANDLER;

                break;

                

        }

    }

    

    return DONE;
@@ -243,13 +258,12 @@ apr_status_t h2_conn_run(struct h2_ctx *ctx, conn_rec *c) 


apr_status_t h2_conn_pre_close(struct h2_ctx *ctx, conn_rec *c)

{

    apr_status_t status;

    

    status = h2_session_pre_close(h2_ctx_session_get(ctx), async_mpm);

    if (status == APR_SUCCESS) {

        return DONE; /* This is the same, right? */

    h2_session *session = h2_ctx_session_get(ctx);

    if (session) {

        apr_status_t status = h2_session_pre_close(session, async_mpm);

        return (status == APR_SUCCESS)? DONE : status;

    }

    return status;

    return DONE;

}



conn_rec *h2_slave_create(conn_rec *master, int slave_id, apr_pool_t *parent)

modules/http2/h2_h2.c

+2 −1
Original line number Diff line number Diff line
@@ -652,6 +652,7 @@ int h2_h2_process_conn(conn_rec* c) 
            status = h2_conn_setup(ctx, c, NULL);

            ap_log_cerror(APLOG_MARK, APLOG_TRACE1, status, c, "conn_setup");

            if (status != APR_SUCCESS) {

                h2_ctx_clear(c);

                return status;

            }

        }
@@ -674,7 +675,7 @@ static int h2_h2_pre_close_conn(conn_rec *c) 
    ctx = h2_ctx_get(c, 0);

    if (ctx) {

        /* If the session has been closed correctly already, we will not

         * fiond a h2_ctx here. The presence indicates that the session

         * find a h2_ctx here. The presence indicates that the session

         * is still ongoing. */

        return h2_conn_pre_close(ctx, c);

    }

modules/http2/h2_session.c

+167 −135
Original line number Diff line number Diff line
@@ -661,11 +661,13 @@ static apr_status_t h2_session_shutdown(h2_session *session, int error, 
         * we have, but no longer accept new ones. Report the max stream

         * we have received and discard all new ones. */

    }

    

    session->local.accepting = 0;

    session->local.shutdown = 1;

    if (!session->c->aborted) {

        nghttp2_submit_goaway(session->ngh2, NGHTTP2_FLAG_NONE, 

                              session->local.accepted_max, 

                              error, (uint8_t*)msg, msg? strlen(msg):0);

    session->local.accepting = 0;

    session->local.shutdown = 1;

        status = nghttp2_session_send(session->ngh2);

        if (status == APR_SUCCESS) {

            status = h2_conn_io_flush(&session->io);
@@ -673,14 +675,15 @@ static apr_status_t h2_session_shutdown(h2_session *session, int error, 
        ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, session->c, 

                      H2_SSSN_LOG(APLOGNO(03069), session, 

                                  "sent GOAWAY, err=%d, msg=%s"), error, msg? msg : "");

    }

    dispatch_event(session, H2_SESSION_EV_LOCAL_GOAWAY, error, msg);

    return status;

}



static apr_status_t session_pool_cleanup(void *data)

static apr_status_t session_cleanup(h2_session *session, const char *trigger)

{

    h2_session *session = data;

    ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, session->c,

    conn_rec *c = session->c;

    ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, c,

                  H2_SSSN_MSG(session, "pool_cleanup"));

    

    if (session->state != H2_SESSION_ST_DONE
@@ -693,13 +696,13 @@ static apr_status_t session_pool_cleanup(void *data) 
         * connection when sending the next request, this has the effect

         * that at least this one request will fail.

         */

        ap_log_cerror(APLOG_MARK, APLOG_WARNING, 0, session->c,

        ap_log_cerror(APLOG_MARK, APLOG_WARNING, 0, c,

                      H2_SSSN_LOG(APLOGNO(03199), session, 

                      "connection disappeared without proper "

                      "goodbye, clients will be confused, should not happen"));

    }



    transit(session, "pool cleanup", H2_SESSION_ST_CLEANUP);

    transit(session, trigger, H2_SESSION_ST_CLEANUP);

    h2_mplx_set_consumed_cb(session->mplx, NULL, NULL);

    h2_mplx_release_and_join(session->mplx, session->iowait);

    session->mplx = NULL;
@@ -707,11 +710,35 @@ static apr_status_t session_pool_cleanup(void *data) 
    ap_assert(session->ngh2);

    nghttp2_session_del(session->ngh2);

    session->ngh2 = NULL;

    h2_ctx_clear(c);

    

    return APR_SUCCESS;

}



static apr_status_t session_pool_cleanup(void *data)

{

    conn_rec *c = data;

    h2_session *session;

    h2_ctx *ctx = h2_ctx_get(c, 0);

    

    if (ctx && (session = h2_ctx_session_get(ctx))) {

        /* if the session is still there, now is the last chance

         * to perform cleanup. Normally, cleanup should have happened

         * earlier in the connection pre_close. Main reason is that

         * any ongoing requests on slave connections might still access

         * data which has, at this time, already been freed. An example

         * is mod_ssl that uses request hooks. */

        ap_log_cerror(APLOG_MARK, APLOG_WARNING, 0, c,

                      H2_SSSN_LOG(APLOGNO(), session, 

                      "session cleanup triggered by pool cleanup. "

                      "this should have happened earlier already."));

        return session_cleanup(session, "pool cleanup");

    }

    return APR_SUCCESS;

}



static h2_session *h2_session_create_int(conn_rec *c,

static apr_status_t h2_session_create_int(h2_session **psession,

                                          conn_rec *c,

                                          request_rec *r,

                                          h2_ctx *ctx, 

                                          h2_workers *workers)
@@ -723,31 +750,35 @@ static h2_session *h2_session_create_int(conn_rec *c, 
    uint32_t n;

    apr_pool_t *pool = NULL;

    h2_session *session;

    apr_status_t status;

    int rv;



    apr_status_t status = apr_allocator_create(&allocator);

    *psession = NULL;

    status = apr_allocator_create(&allocator);

    if (status != APR_SUCCESS) {

        return NULL;

        return status;

    }

    apr_allocator_max_free_set(allocator, ap_max_mem_free);

    apr_pool_create_ex(&pool, c->pool, NULL, allocator);

    if (!pool) {

        apr_allocator_destroy(allocator);

        return NULL;

        return APR_ENOMEM;

    }

    apr_pool_tag(pool, "h2_session");

    apr_allocator_owner_set(allocator, pool);

    status = apr_thread_mutex_create(&mutex, APR_THREAD_MUTEX_DEFAULT, pool);

    if (status != APR_SUCCESS) {

        apr_pool_destroy(pool);

        return NULL;

        return APR_ENOMEM;

    }

    apr_allocator_mutex_set(allocator, mutex);

    

    /* get h2_session a lifetime beyond its pool and everything

     * connected to it. */

    session = apr_pcalloc(pool, sizeof(h2_session));

    if (session) {

        int rv;

    if (!session) {

        return APR_ENOMEM;

    }

    

    *psession = session;

    session->id = c->id;

    session->c = c;

    session->r = r;
@@ -760,8 +791,6 @@ static h2_session *h2_session_create_int(conn_rec *c, 
    session->local.accepting = 1;

    session->remote.accepting = 1;

    

        apr_pool_pre_cleanup_register(pool, session, session_pool_cleanup);

        

    session->max_stream_count = h2_config_geti(session->config, 

                                               H2_CONF_MAX_STREAMS);

    session->max_stream_mem = h2_config_geti(session->config,
@@ -769,12 +798,14 @@ static h2_session *h2_session_create_int(conn_rec *c, 
    

    status = apr_thread_cond_create(&session->iowait, session->pool);

    if (status != APR_SUCCESS) {

            return NULL;

        apr_pool_destroy(pool);

        return status;

    }

    

    session->monitor = apr_pcalloc(pool, sizeof(h2_stream_monitor));

    if (session->monitor == NULL) {

            return NULL;

        apr_pool_destroy(pool);

        return status;

    }

    session->monitor->ctx = session;

    session->monitor->on_state_enter = on_stream_state_enter;
@@ -797,7 +828,8 @@ static h2_session *h2_session_create_int(conn_rec *c, 
    if (status != APR_SUCCESS) {

        ap_log_cerror(APLOG_MARK, APLOG_ERR, status, c, APLOGNO(02927) 

                      "nghttp2: error in init_callbacks");

            return NULL;

        apr_pool_destroy(pool);

        return status;

    }

    

    rv = nghttp2_option_new(&options);
@@ -805,7 +837,8 @@ static h2_session *h2_session_create_int(conn_rec *c, 
        ap_log_cerror(APLOG_MARK, APLOG_ERR, APR_EGENERAL, c,

                      APLOGNO(02928) "nghttp2_option_new: %s", 

                      nghttp2_strerror(rv));

            return NULL;

        apr_pool_destroy(pool);

        return status;

    }

    nghttp2_option_set_peer_max_concurrent_streams(

                                                   options, (uint32_t)session->max_stream_count);
@@ -822,7 +855,8 @@ static h2_session *h2_session_create_int(conn_rec *c, 
        ap_log_cerror(APLOG_MARK, APLOG_ERR, APR_EGENERAL, c,

                      APLOGNO(02929) "nghttp2_session_server_new: %s",

                      nghttp2_strerror(rv));

            return NULL;

        apr_pool_destroy(pool);

        return APR_ENOMEM;

    }

    

    n = h2_config_geti(session->config, H2_CONF_PUSH_DIARY_SIZE);
@@ -841,18 +875,21 @@ static h2_session *h2_session_create_int(conn_rec *c, 
                      session->push_diary->dtype, 

                      (int)session->push_diary->N);

    }

    }

    return session;

    

    apr_pool_pre_cleanup_register(pool, c, session_pool_cleanup);    

    return APR_SUCCESS;

}



h2_session *h2_session_create(conn_rec *c, h2_ctx *ctx, h2_workers *workers)

apr_status_t h2_session_create(h2_session **psession, 

                               conn_rec *c, h2_ctx *ctx, h2_workers *workers)

{

    return h2_session_create_int(c, NULL, ctx, workers);

    return h2_session_create_int(psession, c, NULL, ctx, workers);

}



h2_session *h2_session_rcreate(request_rec *r, h2_ctx *ctx, h2_workers *workers)

apr_status_t h2_session_rcreate(h2_session **psession, 

                                request_rec *r, h2_ctx *ctx, h2_workers *workers)

{

    return h2_session_create_int(r->connection, r, ctx, workers);

    return h2_session_create_int(psession, r->connection, r, ctx, workers);

}



static apr_status_t h2_session_start(h2_session *session, int *rv)
@@ -1919,7 +1956,6 @@ apr_status_t h2_session_process(h2_session *session, int async) 
    }

                  

    while (session->state != H2_SESSION_ST_DONE) {

        trace = APLOGctrace3(c);

        session->have_read = session->have_written = 0;



        if (session->local.accepting
@@ -1957,8 +1993,6 @@ apr_status_t h2_session_process(h2_session *session, int async) 
                break;

                

            case H2_SESSION_ST_IDLE:

                /* make certain, we send everything before we idle */

                h2_conn_io_flush(&session->io);

                /* We trust our connection into the default timeout/keepalive

                 * handling of the core filters/mpm iff:

                 * - keep_sync_until is not set
@@ -1975,6 +2009,7 @@ apr_status_t h2_session_process(h2_session *session, int async) 
                                      "nonblock read, %d streams open"), 

                                      session->open_streams);

                    }

                    h2_conn_io_flush(&session->io);

                    status = h2_session_read(session, 0);

                    

                    if (status == APR_SUCCESS) {
@@ -2001,6 +2036,8 @@ apr_status_t h2_session_process(h2_session *session, int async) 
                    }

                }

                else {

                    /* make certain, we send everything before we idle */

                    h2_conn_io_flush(&session->io);

                    if (trace) {

                        ap_log_cerror(APLOG_MARK, APLOG_TRACE3, status, c,

                                      H2_SSSN_MSG(session,
@@ -2187,12 +2224,7 @@ out: 
        dispatch_event(session, H2_SESSION_EV_CONN_ERROR, 0, NULL);

    }



    status = APR_SUCCESS;

    if (session->state == H2_SESSION_ST_DONE) {

        status = APR_EOF;

    }

    

    return status;

    return (session->state == H2_SESSION_ST_DONE)? APR_EOF : APR_SUCCESS;

}



apr_status_t h2_session_pre_close(h2_session *session, int async)
@@ -2201,5 +2233,5 @@ apr_status_t h2_session_pre_close(h2_session *session, int async) 
                  H2_SSSN_MSG(session, "pre_close"));

    dispatch_event(session, H2_SESSION_EV_PRE_CLOSE, 0, 

        (session->state == H2_SESSION_ST_IDLE)? "timeout" : NULL);

    return APR_SUCCESS;

    return session_cleanup(session, "pre_close");

}

modules/http2/h2_session.h

+8 −4
Original line number Diff line number Diff line
@@ -132,23 +132,27 @@ const char *h2_session_state_str(h2_session_state state); 
/**

 * Create a new h2_session for the given connection.

 * The session will apply the configured parameter.

 * @param psession pointer receiving the created session on success or NULL

 * @param c       the connection to work on

 * @param cfg     the module config to apply

 * @param workers the worker pool to use

 * @return the created session

 */

h2_session *h2_session_create(conn_rec *c, struct h2_ctx *ctx, 

apr_status_t h2_session_create(h2_session **psession,

                               conn_rec *c, struct h2_ctx *ctx, 

                               struct h2_workers *workers);



/**

 * Create a new h2_session for the given request.

 * The session will apply the configured parameter.

 * @param psession pointer receiving the created session on success or NULL

 * @param r       the request that was upgraded

 * @param cfg     the module config to apply

 * @param workers the worker pool to use

 * @return the created session

 */

h2_session *h2_session_rcreate(request_rec *r, struct h2_ctx *ctx,

apr_status_t h2_session_rcreate(h2_session **psession,

                                request_rec *r, struct h2_ctx *ctx,

                                struct h2_workers *workers);



/**