From 456c6cec1ff942d4e3756319211de6355f54b40d Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Wed, 19 Apr 2017 13:03:01 -0700 Subject: [PATCH] Clamp write size to (hopefully) limit buffer bloat --- .../chttp2/transport/chttp2_transport.c | 25 +++++++++++++------ .../ext/transport/chttp2/transport/internal.h | 11 +++++--- .../ext/transport/chttp2/transport/writing.c | 24 +++++++++++++++--- 3 files changed, 45 insertions(+), 15 deletions(-) diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.c b/src/core/ext/transport/chttp2/transport/chttp2_transport.c index 1daca437b27..ce5e3cd20a1 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.c +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.c @@ -884,14 +884,23 @@ static void write_action_begin_locked(grpc_exec_ctx *exec_ctx, void *gt, GPR_TIMER_BEGIN("write_action_begin_locked", 0); grpc_chttp2_transport *t = gt; GPR_ASSERT(t->write_state != GRPC_CHTTP2_WRITE_STATE_IDLE); - if (!t->closed && grpc_chttp2_begin_write(exec_ctx, t)) { - set_write_state(exec_ctx, t, GRPC_CHTTP2_WRITE_STATE_WRITING, - "begin writing"); - grpc_closure_sched(exec_ctx, &t->write_action, GRPC_ERROR_NONE); - } else { - set_write_state(exec_ctx, t, GRPC_CHTTP2_WRITE_STATE_IDLE, - "begin writing nothing"); - GRPC_CHTTP2_UNREF_TRANSPORT(exec_ctx, t, "writing"); + switch (t->closed ? GRPC_CHTTP2_NOTHING_TO_WRITE + : grpc_chttp2_begin_write(exec_ctx, t)) { + case GRPC_CHTTP2_NOTHING_TO_WRITE: + set_write_state(exec_ctx, t, GRPC_CHTTP2_WRITE_STATE_IDLE, + "begin writing nothing"); + GRPC_CHTTP2_UNREF_TRANSPORT(exec_ctx, t, "writing"); + break; + case GRPC_CHTTP2_PARTIAL_WRITE: + set_write_state(exec_ctx, t, GRPC_CHTTP2_WRITE_STATE_WRITING_WITH_MORE, + "begin writing partial"); + grpc_closure_sched(exec_ctx, &t->write_action, GRPC_ERROR_NONE); + break; + case GRPC_CHTTP2_FULL_WRITE: + set_write_state(exec_ctx, t, GRPC_CHTTP2_WRITE_STATE_WRITING, + "begin writing"); + grpc_closure_sched(exec_ctx, &t->write_action, GRPC_ERROR_NONE); + break; } GPR_TIMER_END("write_action_begin_locked", 0); } diff --git a/src/core/ext/transport/chttp2/transport/internal.h b/src/core/ext/transport/chttp2/transport/internal.h index 0aaa4aebe5f..733e9900ef1 100644 --- a/src/core/ext/transport/chttp2/transport/internal.h +++ b/src/core/ext/transport/chttp2/transport/internal.h @@ -552,9 +552,14 @@ void grpc_chttp2_initiate_write(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t, bool covered_by_poller, const char *reason); -/** Someone is unlocking the transport mutex: check to see if writes - are required, and frame them if so */ -bool grpc_chttp2_begin_write(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t); +typedef enum { + GRPC_CHTTP2_NOTHING_TO_WRITE, + GRPC_CHTTP2_PARTIAL_WRITE, + GRPC_CHTTP2_FULL_WRITE, +} grpc_chttp2_begin_write_result; + +grpc_chttp2_begin_write_result grpc_chttp2_begin_write( + grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t); void grpc_chttp2_end_write(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t, grpc_error *error); diff --git a/src/core/ext/transport/chttp2/transport/writing.c b/src/core/ext/transport/chttp2/transport/writing.c index 9267544a5cf..9b4187341d2 100644 --- a/src/core/ext/transport/chttp2/transport/writing.c +++ b/src/core/ext/transport/chttp2/transport/writing.c @@ -168,8 +168,8 @@ uint32_t grpc_chttp2_target_incoming_window(grpc_chttp2_transport *t) { [GRPC_CHTTP2_SETTINGS_INITIAL_WINDOW_SIZE]); } -bool grpc_chttp2_begin_write(grpc_exec_ctx *exec_ctx, - grpc_chttp2_transport *t) { +grpc_chttp2_begin_write_result grpc_chttp2_begin_write( + grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t) { grpc_chttp2_stream *s; GPR_TIMER_BEGIN("grpc_chttp2_begin_write", 0); @@ -203,9 +203,23 @@ bool grpc_chttp2_begin_write(grpc_exec_ctx *exec_ctx, } } + bool partial_write = false; + /* for each grpc_chttp2_stream that's become writable, frame it's data (according to available window sizes) and add to the output buffer */ - while (grpc_chttp2_list_pop_writable_stream(t, &s)) { + while (true) { + if (t->outbuf.length > + GPR_CLAMP(t->settings[GRPC_SENT_SETTINGS] + [GRPC_CHTTP2_SETTINGS_INITIAL_WINDOW_SIZE], + 1024, 1024 * 1024)) { + partial_write = true; + break; + } + + if (!grpc_chttp2_list_pop_writable_stream(t, &s)) { + break; + } + bool sent_initial_metadata = s->sent_initial_metadata; bool now_writing = false; @@ -392,7 +406,9 @@ bool grpc_chttp2_begin_write(grpc_exec_ctx *exec_ctx, GPR_TIMER_END("grpc_chttp2_begin_write", 0); - return t->outbuf.count > 0; + return t->outbuf.count > 0 ? (partial_write ? GRPC_CHTTP2_PARTIAL_WRITE + : GRPC_CHTTP2_FULL_WRITE) + : GRPC_CHTTP2_NOTHING_TO_WRITE; } void grpc_chttp2_end_write(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t,