Skip to content

Commit 6defafb

Browse files
covenercovener
committed
careful handling of session bbtmp
git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1938664 13f79535-47bb-0310-9956-ffa450edef68
1 parent 616ddb1 commit 6defafb

5 files changed

Lines changed: 54 additions & 24 deletions

File tree

‎modules/http2/h2_c1_io.c‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -456,11 +456,15 @@ static apr_status_t c1_in_feed_brigade(h2_session *session,
456456
*inout_len = 0;
457457
while (!APR_BRIGADE_EMPTY(bb)) {
458458
b = APR_BRIGADE_FIRST(bb);
459+
APR_BUCKET_REMOVE(b);
459460
if (!APR_BUCKET_IS_METADATA(b)) {
460461
rv = c1_in_feed_bucket(session, b, inout_len);
461-
if (APR_SUCCESS != rv) goto cleanup;
462+
if (APR_SUCCESS != rv) {
463+
apr_bucket_destroy(b);
464+
goto cleanup;
465+
}
462466
}
463-
apr_bucket_delete(b);
467+
apr_bucket_destroy(b);
464468
}
465469
cleanup:
466470
apr_brigade_cleanup(bb);

‎modules/http2/h2_mplx.c‎

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -148,7 +148,13 @@ static void c1c2_stream_joined(h2_mplx *m, h2_stream *stream)
148148

149149
static void m_stream_cleanup(h2_mplx *m, h2_stream *stream)
150150
{
151-
h2_conn_ctx_t *c2_ctx = h2_conn_ctx_get(stream->c2);
151+
h2_conn_ctx_t *c2_ctx;
152+
153+
if (stream->mplx_cleanup_done) return;
154+
stream->mplx_cleanup_done = 1;
155+
m->in_stream_cleanup = 1;
156+
157+
c2_ctx = h2_conn_ctx_get(stream->c2);
152158

153159
ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, m->c1,
154160
H2_STRM_MSG(stream, "cleanup, unsubscribing from beam events"));
@@ -195,6 +201,7 @@ static void m_stream_cleanup(h2_mplx *m, h2_stream *stream)
195201
ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, m->c1,
196202
H2_STRM_MSG(stream, "cleanup, never started, move to spurge"));
197203
}
204+
m->in_stream_cleanup = 0;
198205
}
199206

200207
static h2_c2_transit *c2_transit_create(h2_mplx *m)
@@ -278,6 +285,7 @@ h2_mplx *h2_mplx_c1_create(int child_num, apr_uint32_t id, h2_stream *stream0,
278285
apr_allocator_t *allocator;
279286
apr_thread_mutex_t *mutex = NULL;
280287
h2_mplx *m = NULL;
288+
apr_pool_t *poll_pool;
281289

282290
m = apr_pcalloc(parent, sizeof(h2_mplx));
283291
m->stream0 = stream0;
@@ -337,8 +345,14 @@ h2_mplx *h2_mplx_c1_create(int child_num, apr_uint32_t id, h2_stream *stream0,
337345
m->streams_ev_in = apr_array_make(m->pool, 10, sizeof(h2_stream*));
338346
m->streams_ev_out = apr_array_make(m->pool, 10, sizeof(h2_stream*));
339347

340-
m->streams_input_read = h2_iq_create(m->pool, 10);
341-
m->streams_output_written = h2_iq_create(m->pool, 10);
348+
/* iqueues MUST use a separate pool: iq_grow allocates via apr_pcalloc
349+
* under poll_lock, while other m->pool allocations (spurge growth etc.)
350+
* happen under m->lock. apr_palloc is NOT thread-safe on the same pool
351+
* under different locks -- the bump pointer can hand out overlapping
352+
* memory to concurrent callers. */
353+
apr_pool_create(&poll_pool, m->pool);
354+
m->streams_input_read = h2_iq_create(poll_pool, m->max_streams);
355+
m->streams_output_written = h2_iq_create(poll_pool, m->max_streams);
342356
status = apr_thread_mutex_create(&m->poll_lock, APR_THREAD_MUTEX_DEFAULT,
343357
m->pool);
344358
if (APR_SUCCESS != status) goto failure;
@@ -500,6 +514,7 @@ static int m_stream_cancel_iter(void *ctx, void *val) {
500514
/* Reset, should transit to CLOSED state */
501515
h2_stream_rst(stream, H2_ERR_NO_ERROR);
502516
/* All connection data has been sent, simulate cleanup */
517+
m->in_stream_cleanup = 1;
503518
h2_stream_dispatch(stream, H2_SEV_EOS_SENT);
504519
m_stream_cleanup(m, stream);
505520
return 0;
@@ -761,7 +776,7 @@ void h2_mplx_c1_process(h2_mplx *m,
761776

762777
while ((sid = h2_iq_shift(ready_to_process)) > 0) {
763778
h2_stream *stream = get_stream(session, sid);
764-
if (stream) {
779+
if (stream && stream->state < H2_SS_CLEANUP) {
765780
ap_assert(!stream->scheduled);
766781
rv = c1_process_stream(session->mplx, stream, stream_pri_cmp, session);
767782
if (APR_SUCCESS != rv) {
@@ -1185,6 +1200,7 @@ apr_status_t h2_mplx_c1_client_rst(h2_mplx *m, int stream_id, h2_stream *stream)
11851200
H2_STRM_MSG(stream, "very early RST, drop"));
11861201
h2_stream_set_monitor(stream, NULL);
11871202
h2_stream_rst(stream, H2_ERR_STREAM_CLOSED);
1203+
m->in_stream_cleanup = 1;
11881204
h2_stream_dispatch(stream, H2_SEV_EOS_SENT);
11891205
m_stream_cleanup(m, stream);
11901206
m_be_annoyed(m);

‎modules/http2/h2_mplx.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@ struct h2_mplx {
6060
apr_uint32_t id; /* id unique per child */
6161
conn_rec *c1; /* the main connection */
6262
apr_pool_t *pool;
63+
int in_stream_cleanup;
6364
struct h2_stream *stream0; /* HTTP/2's stream 0 */
6465
server_rec *s; /* server for master conn */
6566

‎modules/http2/h2_session.c‎

Lines changed: 26 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ static int rst_unprocessed_stream(h2_stream *stream, void *ctx)
9999
(!stream->session->remote.accepting
100100
&& stream->id > stream->session->remote.accepted_max))
101101
);
102-
if (unprocessed) {
102+
if (unprocessed && stream->state < H2_SS_CLEANUP) {
103103
h2_stream_rst(stream, H2_ERR_NO_ERROR);
104104
return 0;
105105
}
@@ -269,7 +269,7 @@ static int on_stream_close_cb(nghttp2_session *ngh2, int32_t stream_id,
269269

270270
(void)ngh2;
271271
stream = get_stream(session, stream_id);
272-
if (stream) {
272+
if (stream && stream->state < H2_SS_CLEANUP) {
273273
if (error_code) {
274274
ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, session->c1,
275275
H2_STRM_LOG(APLOGNO(03065), stream,
@@ -512,6 +512,7 @@ static int on_send_data_cb(nghttp2_session *ngh2,
512512
h2_stream *stream;
513513
apr_bucket *b;
514514
apr_off_t len = length;
515+
apr_bucket_brigade *bb;
515516

516517
(void)ngh2;
517518
(void)source;
@@ -542,29 +543,31 @@ static int on_send_data_cb(nghttp2_session *ngh2,
542543
return NGHTTP2_ERR_CALLBACK_FAILURE;
543544
}
544545

545-
status = h2_stream_read_to(stream, session->bbtmp, &len, &eos);
546+
bb = apr_brigade_create(session->pool,
547+
session->c1->bucket_alloc);
548+
status = h2_stream_read_to(stream, bb, &len, &eos);
546549
if (status != APR_SUCCESS) {
547550
ap_log_cerror(APLOG_MARK, APLOG_TRACE1, status, session->c1,
548551
H2_STRM_MSG(stream, "send_data_cb, reading stream"));
549-
apr_brigade_cleanup(session->bbtmp);
552+
apr_brigade_destroy(bb);
550553
return NGHTTP2_ERR_CALLBACK_FAILURE;
551554
}
552555
else if (len != (apr_off_t)length) {
553556
ap_log_cerror(APLOG_MARK, APLOG_TRACE1, status, session->c1,
554557
H2_STRM_MSG(stream, "send_data_cb, wanted %ld bytes, "
555558
"got %ld from stream"), (long)length, (long)len);
556-
apr_brigade_cleanup(session->bbtmp);
559+
apr_brigade_destroy(bb);
557560
return NGHTTP2_ERR_CALLBACK_FAILURE;
558561
}
559-
562+
560563
if (padlen) {
561-
b = apr_bucket_immortal_create(immortal_zeros, padlen,
564+
b = apr_bucket_immortal_create(immortal_zeros, padlen,
562565
session->c1->bucket_alloc);
563-
APR_BRIGADE_INSERT_TAIL(session->bbtmp, b);
566+
APR_BRIGADE_INSERT_TAIL(bb, b);
564567
}
565-
566-
status = h2_c1_io_append(&session->io, session->bbtmp);
567-
apr_brigade_cleanup(session->bbtmp);
568+
569+
status = h2_c1_io_append(&session->io, bb);
570+
apr_brigade_destroy(bb);
568571

569572
if (status == APR_SUCCESS) {
570573
stream->out_data_frames++;
@@ -1708,6 +1711,7 @@ static void ev_stream_open(h2_session *session, h2_stream *stream)
17081711
static void ev_stream_closed(h2_session *session, h2_stream *stream)
17091712
{
17101713
apr_bucket *b;
1714+
apr_bucket_brigade *bb;
17111715

17121716
if (H2_STREAM_CLIENT_INITIATED(stream->id)
17131717
&& (stream->id > session->local.completed_max)) {
@@ -1721,9 +1725,11 @@ static void ev_stream_closed(h2_session *session, h2_stream *stream)
17211725
ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, session->c1,
17221726
H2_STRM_MSG(stream, "adding h2_eos to c1 out"));
17231727
b = h2_bucket_eos_create(session->c1->bucket_alloc, stream);
1724-
APR_BRIGADE_INSERT_TAIL(session->bbtmp, b);
1725-
h2_c1_io_append(&session->io, session->bbtmp);
1726-
apr_brigade_cleanup(session->bbtmp);
1728+
bb = apr_brigade_create(session->pool,
1729+
session->c1->bucket_alloc);
1730+
APR_BRIGADE_INSERT_TAIL(bb, b);
1731+
h2_c1_io_append(&session->io, bb);
1732+
apr_brigade_destroy(bb);
17271733
}
17281734

17291735
static void on_stream_state_enter(void *ctx, h2_stream *stream)
@@ -1761,10 +1767,12 @@ static void on_stream_state_enter(void *ctx, h2_stream *stream)
17611767
break;
17621768
case H2_SS_CLEANUP:
17631769
nghttp2_session_set_stream_user_data(session->ngh2, stream->id, NULL);
1764-
update_child_status(session, SERVER_BUSY_WRITE, "done", stream);
1765-
h2_mplx_c1_stream_cleanup(session->mplx, stream, &session->open_streams);
1766-
stream = NULL;
1767-
++session->streams_done;
1770+
if (!session->mplx->in_stream_cleanup) {
1771+
update_child_status(session, SERVER_BUSY_WRITE, "done", stream);
1772+
h2_mplx_c1_stream_cleanup(session->mplx, stream, &session->open_streams);
1773+
stream = NULL;
1774+
++session->streams_done;
1775+
}
17681776
break;
17691777
default:
17701778
break;

‎modules/http2/h2_stream.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,7 @@ struct h2_stream {
111111
unsigned int aborted : 1; /* was aborted */
112112
unsigned int scheduled : 1; /* stream has been scheduled */
113113
unsigned int input_closed : 1; /* no more request data/trailers coming */
114+
unsigned int mplx_cleanup_done : 1;
114115
unsigned int push_policy; /* which push policy to use for this request */
115116
unsigned int sent_trailers : 1; /* trailers have been submitted */
116117
unsigned int output_eos : 1; /* output EOS in buffer/sent */

0 commit comments

Comments
 (0)