diff --git a/ChangeLog b/ChangeLog index d3cf3627..6c0f53db 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,11 @@ +v1.0.15 +-------------------------------------------------------------------------------- + * fixed busy loops on random connection errors + * fixed connection state for event MPM that shutdown child with 'G' status + * changed default for H2KeepAliveTimeout. For async MPMs (event), will leave + keepalive handling to the MPM. For sync (worker, prefork), will default to + whatever is set for H2Timeout. + v1.0.14 -------------------------------------------------------------------------------- * fixed segfault on connection shutdown diff --git a/configure.ac b/configure.ac index 9e606512..d2bb7e97 100644 --- a/configure.ac +++ b/configure.ac @@ -14,7 +14,7 @@ # AC_PREREQ([2.69]) -AC_INIT([mod_http2], [1.0.14], [stefan.eissing@greenbytes.de]) +AC_INIT([mod_http2], [1.0.15], [stefan.eissing@greenbytes.de]) LT_PREREQ([2.2.6]) LT_INIT() diff --git a/mod_http2/h2_config.c b/mod_http2/h2_config.c index a7b63fd6..1ddb2657 100644 --- a/mod_http2/h2_config.c +++ b/mod_http2/h2_config.c @@ -60,7 +60,7 @@ static h2_config defconf = { 1, /* HTTP/2 server push enabled */ NULL, /* map of content-type to priorities */ 5, /* normal connection timeout */ - 5, /* keepalive timeout */ + -1, /* keepalive timeout */ 0, /* stream timeout */ }; diff --git a/mod_http2/h2_conn.c b/mod_http2/h2_conn.c index 262748fc..29baa6d9 100644 --- a/mod_http2/h2_conn.c +++ b/mod_http2/h2_conn.c @@ -158,47 +158,71 @@ apr_status_t h2_conn_setup(h2_ctx *ctx, conn_rec *c, request_rec *r) static apr_status_t h2_conn_process(h2_ctx *ctx) { h2_session *session; + apr_status_t status; session = h2_ctx_session_get(ctx); if (session->c->cs) { session->c->cs->sense = CONN_SENSE_DEFAULT; } - h2_session_process(session, async_mpm); + status = h2_session_process(session, async_mpm); session->c->keepalive = AP_CONN_KEEPALIVE; if (session->c->cs) { session->c->cs->state = CONN_STATE_WRITE_COMPLETION; } + if (APR_STATUS_IS_EOF(status) + || APR_STATUS_IS_ECONNRESET(status) + || APR_STATUS_IS_ECONNABORTED(status)) { + /* fatal, probably client just closed connection. emergency shutdown */ + /* Make sure this connection gets closed properly. */ + ap_log_cerror( APLOG_MARK, APLOG_DEBUG, 0, session->c, + "h2_session(%ld): aborted", session->id); + session->c->keepalive = AP_CONN_CLOSE; + + h2_ctx_clear(session->c); + h2_session_abort(session, status); + h2_session_eoc_callback(session); + /* hereafter session might be gone */ + return APR_ECONNABORTED; + } + if (session->state == H2_SESSION_ST_CLOSING) { ap_log_cerror( APLOG_MARK, APLOG_DEBUG, 0, session->c, - "h2_session(%ld): done", session->id); + "h2_session(%ld): closing", session->id); /* Make sure this connection gets closed properly. */ - ap_update_child_status_from_conn(session->c->sbh, SERVER_CLOSING, session->c); session->c->keepalive = AP_CONN_CLOSE; + h2_ctx_clear(session->c); h2_session_close(session); /* hereafter session may be gone */ } + else if (session->state == H2_SESSION_ST_ABORTED) { + ap_log_cerror( APLOG_MARK, APLOG_DEBUG, 0, session->c, + "h2_session(%ld): already aborted", session->id); + return APR_ECONNABORTED; + } - return DONE; + return APR_SUCCESS; } apr_status_t h2_conn_run(struct h2_ctx *ctx, conn_rec *c) { int mpm_state = 0; + apr_status_t status; do { - h2_conn_process(ctx); + status = h2_conn_process(ctx); if (ap_mpm_query(AP_MPMQ_MPM_STATE, &mpm_state)) { break; } - } while (!async_mpm + } while (!async_mpm + && status == APR_SUCCESS && c->keepalive == AP_CONN_KEEPALIVE && mpm_state != AP_MPMQ_STOPPING); - return DONE; + return status; } diff --git a/mod_http2/h2_conn_io.c b/mod_http2/h2_conn_io.c index 2c68db59..773d7bbb 100644 --- a/mod_http2/h2_conn_io.c +++ b/mod_http2/h2_conn_io.c @@ -93,9 +93,15 @@ int h2_conn_io_is_buffered(h2_conn_io *io) return io->bufsize > 0; } +typedef struct { + conn_rec *c; + h2_conn_io *io; +} pass_out_ctx; + static apr_status_t pass_out(apr_bucket_brigade *bb, void *ctx) { - h2_conn_io *io = (h2_conn_io*)ctx; + pass_out_ctx *pctx = ctx; + conn_rec *c = pctx->c; apr_status_t status; apr_off_t bblen; @@ -103,19 +109,19 @@ static apr_status_t pass_out(apr_bucket_brigade *bb, void *ctx) return APR_SUCCESS; } - ap_update_child_status(io->connection->sbh, SERVER_BUSY_WRITE, NULL); + ap_update_child_status(c->sbh, SERVER_BUSY_WRITE, NULL); status = apr_brigade_length(bb, 0, &bblen); if (status == APR_SUCCESS) { - ap_log_cerror(APLOG_MARK, APLOG_TRACE4, 0, io->connection, + ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, c, "h2_conn_io(%ld): pass_out brigade %ld bytes", - io->connection->id, (long)bblen); - status = ap_pass_brigade(io->connection->output_filters, bb); - if (status == APR_SUCCESS) { - io->bytes_written += (apr_size_t)bblen; - io->last_write = apr_time_now(); + c->id, (long)bblen); + status = ap_pass_brigade(c->output_filters, bb); + if (status == APR_SUCCESS && pctx->io) { + pctx->io->bytes_written += (apr_size_t)bblen; + pctx->io->last_write = apr_time_now(); } - apr_brigade_cleanup(bb); } + apr_brigade_cleanup(bb); return status; } @@ -169,7 +175,10 @@ apr_status_t h2_conn_io_write(h2_conn_io *io, const char *buf, size_t length) { apr_status_t status = APR_SUCCESS; + pass_out_ctx ctx; + ctx.c = io->connection; + ctx.io = io; io->unflushed = 1; if (io->bufsize > 0) { ap_log_cerror(APLOG_MARK, APLOG_TRACE4, 0, io->connection, @@ -183,8 +192,9 @@ apr_status_t h2_conn_io_write(h2_conn_io *io, while (length > 0 && (status == APR_SUCCESS)) { apr_size_t avail = io->bufsize - io->buflen; if (avail <= 0) { + bucketeer_buffer(io); - status = pass_out(io->output, io); + status = pass_out(io->output, &ctx); io->buflen = 0; } else if (length > avail) { @@ -205,7 +215,7 @@ apr_status_t h2_conn_io_write(h2_conn_io *io, else { ap_log_cerror(APLOG_MARK, APLOG_TRACE4, status, io->connection, "h2_conn_io: writing %ld bytes to brigade", (long)length); - status = apr_brigade_write(io->output, pass_out, io, buf, length); + status = apr_brigade_write(io->output, pass_out, &ctx, buf, length); } return status; @@ -242,9 +252,11 @@ apr_status_t h2_conn_io_consider_flush(h2_conn_io *io) return status; } -static apr_status_t h2_conn_io_flush_int(h2_conn_io *io, int force) +static apr_status_t h2_conn_io_flush_int(h2_conn_io *io, int force, int eoc) { if (io->unflushed || force) { + pass_out_ctx ctx; + if (io->buflen > 0) { /* something in the buffer, put it in the output brigade */ ap_log_cerror(APLOG_MARK, APLOG_TRACE4, 0, io->connection, @@ -262,20 +274,29 @@ static apr_status_t h2_conn_io_flush_int(h2_conn_io *io, int force) "h2_conn_io: flush"); /* Send it out */ io->unflushed = 0; - return pass_out(io->output, io); + + ctx.c = io->connection; + ctx.io = eoc? NULL : io; + return pass_out(io->output, &ctx); /* no more access after this, as we might have flushed an EOC bucket * that de-allocated us all. */ } return APR_SUCCESS; } +apr_status_t h2_conn_io_write_eoc(h2_conn_io *io, apr_bucket *b) +{ + APR_BRIGADE_INSERT_TAIL(io->output, b); + return h2_conn_io_flush_int(io, 1, 1); +} + apr_status_t h2_conn_io_flush(h2_conn_io *io) { - return h2_conn_io_flush_int(io, 1); + return h2_conn_io_flush_int(io, 1, 0); } apr_status_t h2_conn_io_pass(h2_conn_io *io) { - return h2_conn_io_flush_int(io, 0); + return h2_conn_io_flush_int(io, 0, 0); } diff --git a/mod_http2/h2_conn_io.h b/mod_http2/h2_conn_io.h index e55f08aa..b11480ba 100644 --- a/mod_http2/h2_conn_io.h +++ b/mod_http2/h2_conn_io.h @@ -60,5 +60,6 @@ apr_status_t h2_conn_io_consider_flush(h2_conn_io *io); apr_status_t h2_conn_io_pass(h2_conn_io *io); apr_status_t h2_conn_io_flush(h2_conn_io *io); +apr_status_t h2_conn_io_write_eoc(h2_conn_io *io, apr_bucket *b); #endif /* defined(__mod_h2__h2_conn_io__) */ diff --git a/mod_http2/h2_mplx.c b/mod_http2/h2_mplx.c index 54c83fdf..eba7cc90 100644 --- a/mod_http2/h2_mplx.c +++ b/mod_http2/h2_mplx.c @@ -264,6 +264,8 @@ apr_status_t h2_mplx_release_and_join(h2_mplx *m, apr_thread_cond_t *wait) workers_unregister(m); status = apr_thread_mutex_lock(m->lock); if (APR_SUCCESS == status) { + int i; + /* disable WINDOW_UPDATE callbacks */ h2_mplx_set_consumed_cb(m, NULL, NULL); while (!h2_io_set_iter(m->stream_ios, stream_done_iter, m)) { @@ -271,14 +273,21 @@ apr_status_t h2_mplx_release_and_join(h2_mplx *m, apr_thread_cond_t *wait) } release(m, 0); - while (m->refs > 0) { + for (i = 0; m->refs > 0; ++i) { + m->join_wait = wait; ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, m->c, "h2_mplx(%ld): release_join, refs=%d, waiting...", m->id, m->refs); - apr_thread_cond_wait(wait, m->lock); + + status = apr_thread_cond_timedwait(wait, m->lock, apr_time_from_sec(2)); + if (APR_STATUS_IS_TIMEUP(status)) { + ap_log_cerror(APLOG_MARK, APLOG_INFO, 0, m->c, + "h2_mplx(%ld): release timeup %d, refs=%d, waiting...", + m->id, i, m->refs); + } } - ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, m->c, + ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, m->c, "h2_mplx(%ld): release_join -> destroy, (#ios=%ld)", m->id, (long)h2_io_set_size(m->stream_ios)); apr_thread_mutex_unlock(m->lock); diff --git a/mod_http2/h2_session.c b/mod_http2/h2_session.c index d867e38c..51b2743c 100644 --- a/mod_http2/h2_session.c +++ b/mod_http2/h2_session.c @@ -788,6 +788,9 @@ static h2_session *h2_session_create_int(conn_rec *c, session->max_stream_mem = h2_config_geti(session->config, H2_CONF_STREAM_MAX_MEM); session->timeout_secs = h2_config_geti(session->config, H2_CONF_TIMEOUT_SECS); session->keepalive_secs = h2_config_geti(session->config, H2_CONF_KEEPALIVE_SECS); + if (session->keepalive_secs <= 0) { + session->keepalive_secs = session->timeout_secs; + } status = apr_thread_cond_create(&session->iowait, session->pool); if (status != APR_SUCCESS) { @@ -878,17 +881,17 @@ void h2_session_eoc_callback(h2_session *session) h2_session_destroy(session); } -static apr_status_t h2_session_abort_int(h2_session *session, int reason) +static apr_status_t h2_session_shutdown(h2_session *session, int reason) { AP_DEBUG_ASSERT(session); session->aborted = 1; - if (session->state != H2_SESSION_ST_CLOSING) { - session->state = H2_SESSION_ST_CLOSING; + if (session->state != H2_SESSION_ST_CLOSING + && session->state != H2_SESSION_ST_ABORTED) { if (session->client_goaway) { /* client sent us a GOAWAY, just terminate */ nghttp2_session_terminate_session(session->ngh2, NGHTTP2_ERR_EOF); ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, session->c, - "session(%ld): closed, GOAWAY from client", session->id); + "session(%ld): shutdown, GOAWAY from client", session->id); } else if (!reason) { nghttp2_submit_goaway(session->ngh2, NGHTTP2_FLAG_NONE, @@ -896,7 +899,7 @@ static apr_status_t h2_session_abort_int(h2_session *session, int reason) reason, NULL, 0); nghttp2_session_send(session->ngh2); ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, session->c, - "session(%ld): closed, no err", session->id); + "session(%ld): shutdown, no err", session->id); } else { const char *err = nghttp2_strerror(reason); @@ -906,39 +909,22 @@ static apr_status_t h2_session_abort_int(h2_session *session, int reason) strlen(err)); nghttp2_session_send(session->ngh2); ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, session->c, - "session(%ld): closed, err=%d '%s'", + "session(%ld): shutdown, err=%d '%s'", session->id, reason, err); } + session->state = H2_SESSION_ST_CLOSING; h2_mplx_abort(session->mplx); } return APR_SUCCESS; } -apr_status_t h2_session_abort(h2_session *session, apr_status_t reason, int rv) +void h2_session_abort(h2_session *session, apr_status_t status) { AP_DEBUG_ASSERT(session); - if (rv == 0) { - rv = NGHTTP2_ERR_PROTO; - switch (reason) { - case APR_ENOMEM: - rv = NGHTTP2_ERR_NOMEM; - break; - case APR_SUCCESS: /* all fine, just... */ - case APR_EOF: /* client closed its end... */ - case APR_TIMEUP: /* got bored waiting... */ - rv = 0; /* ...gracefully shut down */ - break; - case APR_EBADF: /* connection unusable, terminate silently */ - default: - if (APR_STATUS_IS_ECONNABORTED(reason) - || APR_STATUS_IS_ECONNRESET(reason) - || APR_STATUS_IS_EBADF(reason)) { - rv = NGHTTP2_ERR_EOF; - } - break; - } - } - return h2_session_abort_int(session, rv); + ap_log_cerror(APLOG_MARK, APLOG_DEBUG, status, session->c, + "h2_session(%ld): aborting", session->id); + session->state = H2_SESSION_ST_ABORTED; + session->aborted = 1; } static apr_status_t h2_session_start(h2_session *session, int *rv) @@ -1104,16 +1090,23 @@ h2_stream *h2_session_get_stream(h2_session *session, int stream_id) void h2_session_close(h2_session *session) { apr_bucket *b; + conn_rec *c = session->c; + apr_status_t status; AP_DEBUG_ASSERT(session); if (!session->aborted) { - h2_session_abort_int(session, 0); + h2_session_shutdown(session, 0); } h2_session_cleanup(session); - b = h2_bucket_eoc_create(session->c->bucket_alloc, session); - h2_conn_io_writeb(&session->io, b); - h2_conn_io_flush(&session->io); + ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, c, + "h2_session(%ld): writing eoc", c->id); + b = h2_bucket_eoc_create(c->bucket_alloc, session); + status = h2_conn_io_write_eoc(&session->io, b); + if (status != APR_SUCCESS) { + ap_log_cerror(APLOG_MARK, APLOG_ERR, status, c, + "h2_session(%ld): flushed eoc bucket", c->id); + } /* and all is or will be destroyed */ } @@ -1300,7 +1293,7 @@ static apr_status_t submit_response(h2_session *session, h2_stream *stream) if (nghttp2_is_fatal(rv)) { status = APR_EGENERAL; - h2_session_abort_int(session, rv); + h2_session_shutdown(session, rv); ap_log_cerror(APLOG_MARK, APLOG_ERR, status, session->c, APLOGNO(02940) "submit_response: %s", nghttp2_strerror(rv)); @@ -1584,7 +1577,7 @@ static apr_status_t h2_session_send(h2_session *session) if (nghttp2_is_fatal(rv)) { ap_log_cerror( APLOG_MARK, APLOG_DEBUG, 0, session->c, "h2_session: send gave error=%s", nghttp2_strerror(rv)); - h2_session_abort_int(session, rv); + h2_session_shutdown(session, rv); return APR_EGENERAL; } } @@ -1608,7 +1601,7 @@ static apr_status_t h2_session_receive(void *ctx, const char *data, "h2_session: nghttp2_session_mem_recv error=%d", (int)n); if (nghttp2_is_fatal((int)n)) { - h2_session_abort_int(session, (int)n); + h2_session_shutdown(session, (int)n); return APR_EGENERAL; } } @@ -1672,7 +1665,6 @@ static apr_status_t h2_session_read(h2_session *session, int block, int loops) "h2_session(%ld): error reading, terminating", session->id); } - h2_session_abort(session, status, 0); return status; } /* subsequent failure after success(es), return initial @@ -1730,7 +1722,7 @@ apr_status_t h2_session_process(h2_session *session, int async) if (session->aborted) { reason = "aborted"; - status = APR_EOF; + status = APR_ECONNABORTED; goto out; } @@ -1747,8 +1739,6 @@ apr_status_t h2_session_process(h2_session *session, int async) session->s->server_hostname, c->local_addr->port); if (status != APR_SUCCESS) { - h2_session_abort(session, status, rv); - h2_session_eoc_callback(session); reason = "start failed"; goto out; } @@ -1911,10 +1901,10 @@ apr_status_t h2_session_process(h2_session *session, int async) * extend that with the H2KeepAliveTimeout. This works different * for async MPMs. */ remain_secs = session->keepalive_secs - session->timeout_secs; - if (remain_secs <= 0) { - /* keepalive is smaller than normal timeout, close the session */ + if (!async && remain_secs <= 0) { + /* not async, keepalive is smaller than normal timeout, close the session */ reason = "keepalive expired"; - h2_session_abort_int(session, 0); + h2_session_shutdown(session, 0); goto out; } ap_update_child_status_from_conn(c->sbh, SERVER_BUSY_KEEPALIVE, c); @@ -1940,7 +1930,7 @@ apr_status_t h2_session_process(h2_session *session, int async) status = h2_session_read(session, 1, 1); if (APR_STATUS_IS_TIMEUP(status)) { reason = "keepalive expired"; - h2_session_abort_int(session, 0); + h2_session_shutdown(session, 0); goto out; } else if (status != APR_SUCCESS) { @@ -1964,6 +1954,11 @@ apr_status_t h2_session_process(h2_session *session, int async) reason = "closing"; goto out; + case H2_SESSION_ST_ABORTED: + ap_log_cerror(APLOG_MARK, APLOG_DEBUG, status, c, + "h2_session(%ld): processing ABORTED", session->id); + return APR_ECONNABORTED; + default: ap_log_cerror(APLOG_MARK, APLOG_ERR, APR_EGENERAL, c, "h2_session(%ld): state %d", session->id, session->state); diff --git a/mod_http2/h2_session.h b/mod_http2/h2_session.h index 39f27afb..7bf316c1 100644 --- a/mod_http2/h2_session.h +++ b/mod_http2/h2_session.h @@ -60,6 +60,7 @@ typedef enum { H2_SESSION_ST_BUSY_WAIT, /* waiting for tasks reporting back */ H2_SESSION_ST_KEEPALIVE, /* nothing to write, normal timeout passed */ H2_SESSION_ST_CLOSING, /* shuting down */ + H2_SESSION_ST_ABORTED, /* client closed connection or sky fall */ } h2_session_state; typedef struct h2_session { @@ -153,13 +154,12 @@ apr_status_t h2_session_process(h2_session *session, int async); void h2_session_eoc_callback(h2_session *session); /** - * Called when an error occured and the session needs to shut down. - * @param session the session to shut down - * @param reason the apache status that caused the shutdown - * @param rv the nghttp2 reason for shutdown, set to 0 if you have none. - * + * Called when a serious error occured and the session needs to terminate + * without further connection io. + * @param session the session to abort + * @param reason the apache status that caused the abort */ -apr_status_t h2_session_abort(h2_session *session, apr_status_t reason, int rv); +void h2_session_abort(h2_session *session, apr_status_t reason); /** * Close and deallocate the given session. diff --git a/mod_http2/h2_switch.c b/mod_http2/h2_switch.c index 62be2349..c08dd9e5 100644 --- a/mod_http2/h2_switch.c +++ b/mod_http2/h2_switch.c @@ -163,7 +163,8 @@ static int h2_protocol_switch(conn_rec *c, request_rec *r, server_rec *s, return status; } - return h2_conn_run(ctx, c); + h2_conn_run(ctx, c); + return DONE; } return DONE; } diff --git a/mod_http2/h2_version.h b/mod_http2/h2_version.h index 6bfc88f5..1c640276 100644 --- a/mod_http2/h2_version.h +++ b/mod_http2/h2_version.h @@ -16,19 +16,25 @@ #ifndef mod_h2_h2_version_h #define mod_h2_h2_version_h +#undef PACKAGE_VERSION +#undef PACKAGE_TARNAME +#undef PACKAGE_STRING +#undef PACKAGE_NAME +#undef PACKAGE_BUGREPORT + /** * @macro - * Version number of the h2 module as c string + * Version number of the http2 module as c string */ -#define MOD_HTTP2_VERSION "1.0.14" +#define MOD_HTTP2_VERSION "1.0.15" /** * @macro - * Numerical representation of the version number of the h2 module + * Numerical representation of the version number of the http2 module * release. This is a 24 bit number with 8 bits for major number, 8 bits * for minor and 8 bits for patch. Version 1.2.3 becomes 0x010203. */ -#define MOD_HTTP2_VERSION_NUM 0x01000e +#define MOD_HTTP2_VERSION_NUM 0x01000f #endif /* mod_h2_h2_version_h */ diff --git a/mod_http2/h2_workers.c b/mod_http2/h2_workers.c index 89aa4efd..8c162699 100644 --- a/mod_http2/h2_workers.c +++ b/mod_http2/h2_workers.c @@ -272,7 +272,7 @@ h2_workers *h2_workers_create(server_rec *s, apr_pool_t *server_pool, apr_atomic_set32(&workers->max_idle_secs, 10); apr_threadattr_create(&workers->thread_attr, workers->pool); - apr_threadattr_detach_set(workers->thread_attr, 0); + apr_threadattr_detach_set(workers->thread_attr, 1); if (ap_thread_stacksize != 0) { apr_threadattr_stacksize_set(workers->thread_attr, ap_thread_stacksize);