You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by ic...@apache.org on 2019/12/19 09:39:22 UTC
svn commit: r1871810 - in /httpd/httpd/trunk: CHANGES
modules/http2/h2_mplx.c modules/http2/h2_session.c modules/http2/h2_util.c
modules/http2/h2_util.h modules/http2/h2_version.h
modules/http2/h2_workers.c modules/http2/mod_proxy_http2.c
Author: icing
Date: Thu Dec 19 09:39:22 2019
New Revision: 1871810
URL: http://svn.apache.org/viewvc?rev=1871810&view=rev
Log:
*) mod_http2: Fixed rare cases where a h2 worker could deadlock the main connection.
Modified:
httpd/httpd/trunk/CHANGES
httpd/httpd/trunk/modules/http2/h2_mplx.c
httpd/httpd/trunk/modules/http2/h2_session.c
httpd/httpd/trunk/modules/http2/h2_util.c
httpd/httpd/trunk/modules/http2/h2_util.h
httpd/httpd/trunk/modules/http2/h2_version.h
httpd/httpd/trunk/modules/http2/h2_workers.c
httpd/httpd/trunk/modules/http2/mod_proxy_http2.c
Modified: httpd/httpd/trunk/CHANGES
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=1871810&r1=1871809&r2=1871810&view=diff
==============================================================================
--- httpd/httpd/trunk/CHANGES [utf-8] (original)
+++ httpd/httpd/trunk/CHANGES [utf-8] Thu Dec 19 09:39:22 2019
@@ -1,6 +1,9 @@
-*- coding: utf-8 -*-
Changes with Apache 2.5.1
+ *) mod_http2: Fixed rare cases where a h2 worker could deadlock the main connection.
+ [Yann Ylavic, Stefan Eissing]
+
*) mod_lua: Accept nil assignments to the exposed tables (r.subprocess_env,
r.headers_out, etc) to remove the key from the table. PR63971.
[Eric Covener]
Modified: httpd/httpd/trunk/modules/http2/h2_mplx.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_mplx.c?rev=1871810&r1=1871809&r2=1871810&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/http2/h2_mplx.c (original)
+++ httpd/httpd/trunk/modules/http2/h2_mplx.c Thu Dec 19 09:39:22 2019
@@ -75,13 +75,13 @@ apr_status_t h2_mplx_child_init(apr_pool
#define H2_MPLX_ENTER_ALWAYS(m) \
apr_thread_mutex_lock(m->lock)
-#define H2_MPLX_ENTER_MAYBE(m, lock) \
- if (lock) apr_thread_mutex_lock(m->lock)
+#define H2_MPLX_ENTER_MAYBE(m, dolock) \
+ if (dolock) apr_thread_mutex_lock(m->lock)
-#define H2_MPLX_LEAVE_MAYBE(m, lock) \
- if (lock) apr_thread_mutex_unlock(m->lock)
+#define H2_MPLX_LEAVE_MAYBE(m, dolock) \
+ if (dolock) apr_thread_mutex_unlock(m->lock)
-static void check_data_for(h2_mplx *m, h2_stream *stream, int lock);
+static void check_data_for(h2_mplx *m, h2_stream *stream, int mplx_is_locked);
static void stream_output_consumed(void *ctx,
h2_bucket_beam *beam, apr_off_t length)
@@ -104,6 +104,7 @@ static void stream_joined(h2_mplx *m, h2
{
ap_assert(!h2_task_has_started(stream->task) || stream->task->worker_done);
+ h2_ififo_remove(m->readyq, stream->id);
h2_ihash_remove(m->shold, stream->id);
h2_ihash_add(m->spurge, stream);
}
@@ -125,14 +126,16 @@ static void stream_cleanup(h2_mplx *m, h
h2_ihash_remove(m->streams, stream->id);
h2_iq_remove(m->q, stream->id);
- h2_ififo_remove(m->readyq, stream->id);
- h2_ihash_add(m->shold, stream);
if (!h2_task_has_started(stream->task) || stream->task->done_done) {
stream_joined(m, stream);
}
- else if (stream->task) {
- stream->task->c->aborted = 1;
+ else {
+ h2_ififo_remove(m->readyq, stream->id);
+ h2_ihash_add(m->shold, stream);
+ if (stream->task) {
+ stream->task->c->aborted = 1;
+ }
}
}
@@ -508,12 +511,11 @@ static void output_produced(void *ctx, h
h2_stream *stream = ctx;
h2_mplx *m = stream->session->mplx;
- check_data_for(m, stream, 1);
+ check_data_for(m, stream, 0);
}
static apr_status_t out_open(h2_mplx *m, int stream_id, h2_bucket_beam *beam)
{
- apr_status_t status = APR_SUCCESS;
h2_stream *stream = h2_ihash_get(m->streams, stream_id);
if (!stream || !stream->task || m->aborted) {
@@ -527,7 +529,7 @@ static apr_status_t out_open(h2_mplx *m,
h2_beam_log(beam, m->c, APLOG_TRACE2, "out_open");
}
else {
- ap_log_cerror(APLOG_MARK, APLOG_TRACE1, status, m->c,
+ ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, m->c,
"h2_mplx(%s): out open", stream->task->id);
}
@@ -539,8 +541,8 @@ static apr_status_t out_open(h2_mplx *m,
/* we might see some file buckets in the output, see
* if we have enough handles reserved. */
- check_data_for(m, stream, 0);
- return status;
+ check_data_for(m, stream, 1);
+ return APR_SUCCESS;
}
apr_status_t h2_mplx_out_open(h2_mplx *m, int stream_id, h2_bucket_beam *beam)
@@ -582,7 +584,7 @@ static apr_status_t out_close(h2_mplx *m
status = h2_beam_close(task->output.beam);
h2_beam_log(task->output.beam, m->c, APLOG_TRACE2, "out_close");
output_consumed_signal(m, task);
- check_data_for(m, stream, 0);
+ check_data_for(m, stream, 1);
return status;
}
@@ -616,15 +618,23 @@ apr_status_t h2_mplx_out_trywait(h2_mplx
return status;
}
-static void check_data_for(h2_mplx *m, h2_stream *stream, int lock)
+static void check_data_for(h2_mplx *m, h2_stream *stream, int mplx_is_locked)
{
+ /* If m->lock is already held, we must release during h2_ififo_push()
+ * which can wait on its not_full condition, causing a deadlock because
+ * no one would then be able to acquire m->lock to empty the fifo.
+ */
+ H2_MPLX_LEAVE_MAYBE(m, mplx_is_locked);
if (h2_ififo_push(m->readyq, stream->id) == APR_SUCCESS) {
+ H2_MPLX_ENTER_ALWAYS(m);
apr_atomic_set32(&m->event_pending, 1);
- H2_MPLX_ENTER_MAYBE(m, lock);
if (m->added_output) {
apr_thread_cond_signal(m->added_output);
}
- H2_MPLX_LEAVE_MAYBE(m, lock);
+ H2_MPLX_LEAVE_MAYBE(m, !mplx_is_locked);
+ }
+ else {
+ H2_MPLX_ENTER_MAYBE(m, mplx_is_locked);
}
}
@@ -677,7 +687,7 @@ apr_status_t h2_mplx_process(h2_mplx *m,
h2_ihash_add(m->streams, stream);
if (h2_stream_is_ready(stream)) {
/* already have a response */
- check_data_for(m, stream, 0);
+ check_data_for(m, stream, 1);
ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, m->c,
H2_STRM_MSG(stream, "process, add to readyq"));
}
@@ -808,7 +818,7 @@ static void task_done(h2_mplx *m, h2_tas
}
/* more data will not arrive, resume the stream */
- check_data_for(m, stream, 0);
+ check_data_for(m, stream, 1);
}
}
else if ((stream = h2_ihash_get(m->shold, task->stream_id)) != NULL) {
@@ -1060,7 +1070,7 @@ apr_status_t h2_mplx_idle(h2_mplx *m)
h2_beam_is_closed(stream->output),
(long)h2_beam_get_buffered(stream->output));
h2_ihash_add(m->streams, stream);
- check_data_for(m, stream, 0);
+ check_data_for(m, stream, 1);
stream->out_checked = 1;
status = APR_EAGAIN;
}
@@ -1112,7 +1122,7 @@ apr_status_t h2_mplx_dispatch_master_eve
apr_status_t h2_mplx_keep_active(h2_mplx *m, h2_stream *stream)
{
- check_data_for(m, stream, 1);
+ check_data_for(m, stream, 0);
return APR_SUCCESS;
}
Modified: httpd/httpd/trunk/modules/http2/h2_session.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_session.c?rev=1871810&r1=1871809&r2=1871810&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/http2/h2_session.c (original)
+++ httpd/httpd/trunk/modules/http2/h2_session.c Thu Dec 19 09:39:22 2019
@@ -2141,7 +2141,7 @@ apr_status_t h2_session_process(h2_sessi
break;
case H2_SESSION_ST_IDLE:
- if (session->idle_until && (apr_time_now() + session->idle_delay) > session->idle_until) {
+ if (session->idle_until && (now + session->idle_delay) > session->idle_until) {
ap_log_cerror( APLOG_MARK, APLOG_TRACE1, status, c,
H2_SSSN_MSG(session, "idle, timeout reached, closing"));
if (session->idle_delay) {
Modified: httpd/httpd/trunk/modules/http2/h2_util.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_util.c?rev=1871810&r1=1871809&r2=1871810&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/http2/h2_util.c (original)
+++ httpd/httpd/trunk/modules/http2/h2_util.c Thu Dec 19 09:39:22 2019
@@ -638,15 +638,6 @@ apr_status_t h2_fifo_term(h2_fifo *fifo)
apr_status_t rv;
if ((rv = apr_thread_mutex_lock(fifo->lock)) == APR_SUCCESS) {
fifo->aborted = 1;
- apr_thread_mutex_unlock(fifo->lock);
- }
- return rv;
-}
-
-apr_status_t h2_fifo_interrupt(h2_fifo *fifo)
-{
- apr_status_t rv;
- if ((rv = apr_thread_mutex_lock(fifo->lock)) == APR_SUCCESS) {
apr_thread_cond_broadcast(fifo->not_empty);
apr_thread_cond_broadcast(fifo->not_full);
apr_thread_mutex_unlock(fifo->lock);
@@ -710,10 +701,6 @@ static apr_status_t fifo_push(h2_fifo *f
{
apr_status_t rv;
- if (fifo->aborted) {
- return APR_EOF;
- }
-
if ((rv = apr_thread_mutex_lock(fifo->lock)) == APR_SUCCESS) {
rv = fifo_push_int(fifo, elem, block);
apr_thread_mutex_unlock(fifo->lock);
@@ -754,10 +741,6 @@ static apr_status_t fifo_pull(h2_fifo *f
{
apr_status_t rv;
- if (fifo->aborted) {
- return APR_EOF;
- }
-
if ((rv = apr_thread_mutex_lock(fifo->lock)) == APR_SUCCESS) {
rv = pull_head(fifo, pelem, block);
apr_thread_mutex_unlock(fifo->lock);
@@ -946,15 +929,6 @@ apr_status_t h2_ififo_term(h2_ififo *fif
apr_status_t rv;
if ((rv = apr_thread_mutex_lock(fifo->lock)) == APR_SUCCESS) {
fifo->aborted = 1;
- apr_thread_mutex_unlock(fifo->lock);
- }
- return rv;
-}
-
-apr_status_t h2_ififo_interrupt(h2_ififo *fifo)
-{
- apr_status_t rv;
- if ((rv = apr_thread_mutex_lock(fifo->lock)) == APR_SUCCESS) {
apr_thread_cond_broadcast(fifo->not_empty);
apr_thread_cond_broadcast(fifo->not_full);
apr_thread_mutex_unlock(fifo->lock);
@@ -1018,10 +992,6 @@ static apr_status_t ififo_push(h2_ififo
{
apr_status_t rv;
- if (fifo->aborted) {
- return APR_EOF;
- }
-
if ((rv = apr_thread_mutex_lock(fifo->lock)) == APR_SUCCESS) {
rv = ififo_push_int(fifo, id, block);
apr_thread_mutex_unlock(fifo->lock);
@@ -1062,10 +1032,6 @@ static apr_status_t ififo_pull(h2_ififo
{
apr_status_t rv;
- if (fifo->aborted) {
- return APR_EOF;
- }
-
if ((rv = apr_thread_mutex_lock(fifo->lock)) == APR_SUCCESS) {
rv = ipull_head(fifo, pi, block);
apr_thread_mutex_unlock(fifo->lock);
@@ -1088,10 +1054,6 @@ static apr_status_t ififo_peek(h2_ififo
apr_status_t rv;
int id;
- if (fifo->aborted) {
- return APR_EOF;
- }
-
if (APR_SUCCESS == (rv = apr_thread_mutex_lock(fifo->lock))) {
if (APR_SUCCESS == (rv = ipull_head(fifo, &id, block))) {
switch (fn(id, ctx)) {
@@ -1117,39 +1079,40 @@ apr_status_t h2_ififo_try_peek(h2_ififo
return ififo_peek(fifo, fn, ctx, 0);
}
-apr_status_t h2_ififo_remove(h2_ififo *fifo, int id)
+static apr_status_t ififo_remove(h2_ififo *fifo, int id)
{
- apr_status_t rv;
+ int rc, i;
if (fifo->aborted) {
return APR_EOF;
}
- if ((rv = apr_thread_mutex_lock(fifo->lock)) == APR_SUCCESS) {
- int i, rc;
- int e;
-
- rc = 0;
- for (i = 0; i < fifo->count; ++i) {
- e = fifo->elems[inth_index(fifo, i)];
- if (e == id) {
- ++rc;
- }
- else if (rc) {
- fifo->elems[inth_index(fifo, i-rc)] = e;
- }
- }
- if (rc) {
- fifo->count -= rc;
- if (fifo->count + rc == fifo->nelems) {
- apr_thread_cond_broadcast(fifo->not_full);
- }
- rv = APR_SUCCESS;
+ rc = 0;
+ for (i = 0; i < fifo->count; ++i) {
+ int e = fifo->elems[inth_index(fifo, i)];
+ if (e == id) {
+ ++rc;
}
- else {
- rv = APR_EAGAIN;
+ else if (rc) {
+ fifo->elems[inth_index(fifo, i-rc)] = e;
}
-
+ }
+ if (!rc) {
+ return APR_EAGAIN;
+ }
+ fifo->count -= rc;
+ if (fifo->count + rc == fifo->nelems) {
+ apr_thread_cond_broadcast(fifo->not_full);
+ }
+ return APR_SUCCESS;
+}
+
+apr_status_t h2_ififo_remove(h2_ififo *fifo, int id)
+{
+ apr_status_t rv;
+
+ if ((rv = apr_thread_mutex_lock(fifo->lock)) == APR_SUCCESS) {
+ rv = ififo_remove(fifo, id);
apr_thread_mutex_unlock(fifo->lock);
}
return rv;
Modified: httpd/httpd/trunk/modules/http2/h2_util.h
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_util.h?rev=1871810&r1=1871809&r2=1871810&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/http2/h2_util.h (original)
+++ httpd/httpd/trunk/modules/http2/h2_util.h Thu Dec 19 09:39:22 2019
@@ -209,7 +209,6 @@ apr_status_t h2_fifo_create(h2_fifo **pf
apr_status_t h2_fifo_set_create(h2_fifo **pfifo, apr_pool_t *pool, int capacity);
apr_status_t h2_fifo_term(h2_fifo *fifo);
-apr_status_t h2_fifo_interrupt(h2_fifo *fifo);
int h2_fifo_count(h2_fifo *fifo);
@@ -280,7 +279,6 @@ apr_status_t h2_ififo_create(h2_ififo **
apr_status_t h2_ififo_set_create(h2_ififo **pfifo, apr_pool_t *pool, int capacity);
apr_status_t h2_ififo_term(h2_ififo *fifo);
-apr_status_t h2_ififo_interrupt(h2_ififo *fifo);
int h2_ififo_count(h2_ififo *fifo);
Modified: httpd/httpd/trunk/modules/http2/h2_version.h
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_version.h?rev=1871810&r1=1871809&r2=1871810&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/http2/h2_version.h (original)
+++ httpd/httpd/trunk/modules/http2/h2_version.h Thu Dec 19 09:39:22 2019
@@ -27,7 +27,7 @@
* @macro
* Version number of the http2 module as c string
*/
-#define MOD_HTTP2_VERSION "1.15.4"
+#define MOD_HTTP2_VERSION "1.15.5"
/**
* @macro
@@ -35,7 +35,7 @@
* 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 0x010f04
+#define MOD_HTTP2_VERSION_NUM 0x010f05
#endif /* mod_h2_h2_version_h */
Modified: httpd/httpd/trunk/modules/http2/h2_workers.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_workers.c?rev=1871810&r1=1871809&r2=1871810&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/http2/h2_workers.c (original)
+++ httpd/httpd/trunk/modules/http2/h2_workers.c Thu Dec 19 09:39:22 2019
@@ -269,7 +269,6 @@ static apr_status_t workers_pool_cleanup
}
h2_fifo_term(workers->mplxs);
- h2_fifo_interrupt(workers->mplxs);
cleanup_zombies(workers);
}
Modified: httpd/httpd/trunk/modules/http2/mod_proxy_http2.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/mod_proxy_http2.c?rev=1871810&r1=1871809&r2=1871810&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/http2/mod_proxy_http2.c (original)
+++ httpd/httpd/trunk/modules/http2/mod_proxy_http2.c Thu Dec 19 09:39:22 2019
@@ -403,6 +403,14 @@ run_connect:
*/
apr_table_setn(ctx->p_conn->connection->notes,
"proxy-request-alpn-protos", "h2");
+ if (ctx->p_conn->ssl_hostname) {
+ ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, ctx->owner,
+ "set SNI to %s for (%s)",
+ ctx->p_conn->ssl_hostname,
+ ctx->p_conn->hostname);
+ apr_table_setn(ctx->p_conn->connection->notes,
+ "proxy-request-hostname", ctx->p_conn->ssl_hostname);
+ }
}
if (ctx->master->aborted) goto cleanup;
Re: svn commit: r1871810 - in /httpd/httpd/trunk: CHANGES
modules/http2/h2_mplx.c modules/http2/h2_session.c modules/http2/h2_util.c
modules/http2/h2_util.h modules/http2/h2_version.h modules/http2/h2_workers.c
modules/http2/mod_proxy_http2.c
Posted by Stefan Eissing <st...@greenbytes.de>.
> Am 19.12.2019 um 13:19 schrieb Yann Ylavic <yl...@gmail.com>:
>
> On Thu, Dec 19, 2019 at 1:04 PM Stefan Eissing
> <st...@greenbytes.de> wrote:
>>
>> Where do you see this callback invoked when the mplx is locked?
>
> As I said, h2_mplx_out_open() takes the lock, calls out_open() which
> calls h2_beam_on_produced(stream->output, output_produced, stream).
> But actually it seems that h2_beam_on_produced() only sets the
> callback, it does not run it, so it was a too simplistic analysis of
> mine probably... Sorry for the noise.
No, please make noise! There is always something wrong with my assumptions. Someone needs to shake them!
;-)
Re: svn commit: r1871810 - in /httpd/httpd/trunk: CHANGES
modules/http2/h2_mplx.c modules/http2/h2_session.c modules/http2/h2_util.c
modules/http2/h2_util.h modules/http2/h2_version.h modules/http2/h2_workers.c modules/http2/mod_proxy_http2.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Thu, Dec 19, 2019 at 1:04 PM Stefan Eissing
<st...@greenbytes.de> wrote:
>
> Where do you see this callback invoked when the mplx is locked?
As I said, h2_mplx_out_open() takes the lock, calls out_open() which
calls h2_beam_on_produced(stream->output, output_produced, stream).
But actually it seems that h2_beam_on_produced() only sets the
callback, it does not run it, so it was a too simplistic analysis of
mine probably... Sorry for the noise.
Re: svn commit: r1871810 - in /httpd/httpd/trunk: CHANGES
modules/http2/h2_mplx.c modules/http2/h2_session.c modules/http2/h2_util.c
modules/http2/h2_util.h modules/http2/h2_version.h modules/http2/h2_workers.c
modules/http2/mod_proxy_http2.c
Posted by Stefan Eissing <st...@greenbytes.de>.
> Am 19.12.2019 um 12:59 schrieb Yann Ylavic <yl...@gmail.com>:
>
> On Thu, Dec 19, 2019 at 10:39 AM <ic...@apache.org> wrote:
> []
>>
>> Modified: httpd/httpd/trunk/modules/http2/h2_mplx.c
>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_mplx.c?rev=1871810&r1=1871809&r2=1871810&view=diff
>> ==============================================================================
>> --- httpd/httpd/trunk/modules/http2/h2_mplx.c (original)
>> +++ httpd/httpd/trunk/modules/http2/h2_mplx.c Thu Dec 19 09:39:22 2019
> []
>>
>> @@ -508,12 +511,11 @@ static void output_produced(void *ctx, h
>> h2_stream *stream = ctx;
>> h2_mplx *m = stream->session->mplx;
>>
>> - check_data_for(m, stream, 1);
>> + check_data_for(m, stream, 0);
>> }
>
> This should be mplx_is_locked=1, no? It seems that h2_mplx_out_open()
> takes the lock, then calls out_open() => output_produced().
Hmm. The output_produced is a callback in the h2_task output beam. The idea is that when the h2 request writes to its output brigade, which ends up in the output beam, this callback notifies the h2_mplx that there is data to be handled.
Where do you see this callback invoked when the mplx is locked?
Re: svn commit: r1871810 - in /httpd/httpd/trunk: CHANGES
modules/http2/h2_mplx.c modules/http2/h2_session.c modules/http2/h2_util.c
modules/http2/h2_util.h modules/http2/h2_version.h modules/http2/h2_workers.c modules/http2/mod_proxy_http2.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Thu, Dec 19, 2019 at 10:39 AM <ic...@apache.org> wrote:
[]
>
> Modified: httpd/httpd/trunk/modules/http2/h2_mplx.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_mplx.c?rev=1871810&r1=1871809&r2=1871810&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/http2/h2_mplx.c (original)
> +++ httpd/httpd/trunk/modules/http2/h2_mplx.c Thu Dec 19 09:39:22 2019
[]
>
> @@ -508,12 +511,11 @@ static void output_produced(void *ctx, h
> h2_stream *stream = ctx;
> h2_mplx *m = stream->session->mplx;
>
> - check_data_for(m, stream, 1);
> + check_data_for(m, stream, 0);
> }
This should be mplx_is_locked=1, no? It seems that h2_mplx_out_open()
takes the lock, then calls out_open() => output_produced().