From fc2636d7a66cc39596dbaebb1813adc93191290d Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Mon, 12 Sep 2016 09:57:07 -0700 Subject: [PATCH] Fix resource leak --- .../chttp2/transport/chttp2_transport.c | 32 ++++++++++++------- src/core/lib/iomgr/combiner.c | 23 +++++++++---- src/core/lib/iomgr/iomgr.c | 11 +++++-- src/core/lib/support/log.c | 3 +- test/core/end2end/tests/payload.c | 6 ++-- 5 files changed, 50 insertions(+), 25 deletions(-) diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.c b/src/core/ext/transport/chttp2/transport/chttp2_transport.c index ff883dbd763..88ae81a35dc 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.c +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.c @@ -564,11 +564,11 @@ static const char *write_state_name(grpc_chttp2_write_state st) { } static void set_write_state(grpc_chttp2_transport *t, - grpc_chttp2_write_state st) { - GRPC_CHTTP2_IF_TRACING(gpr_log(GPR_DEBUG, "W:%p %s state %s -> %s", t, + grpc_chttp2_write_state st, const char *reason) { + GRPC_CHTTP2_IF_TRACING(gpr_log(GPR_DEBUG, "W:%p %s state %s -> %s [%s]", t, t->is_client ? "CLIENT" : "SERVER", write_state_name(t->write_state), - write_state_name(st))); + write_state_name(st), reason)); t->write_state = st; } @@ -579,7 +579,7 @@ void grpc_chttp2_initiate_write(grpc_exec_ctx *exec_ctx, switch (t->write_state) { case GRPC_CHTTP2_WRITE_STATE_IDLE: - set_write_state(t, GRPC_CHTTP2_WRITE_STATE_WRITING); + set_write_state(t, GRPC_CHTTP2_WRITE_STATE_WRITING, reason); GRPC_CHTTP2_REF_TRANSPORT(t, "writing"); grpc_combiner_execute_finally(exec_ctx, t->combiner, &t->write_action_begin_locked, @@ -590,12 +590,14 @@ void grpc_chttp2_initiate_write(grpc_exec_ctx *exec_ctx, t, covered_by_poller ? GRPC_CHTTP2_WRITE_STATE_WRITING_WITH_MORE_AND_COVERED_BY_POLLER - : GRPC_CHTTP2_WRITE_STATE_WRITING_WITH_MORE); + : GRPC_CHTTP2_WRITE_STATE_WRITING_WITH_MORE, + reason); break; case GRPC_CHTTP2_WRITE_STATE_WRITING_WITH_MORE: if (covered_by_poller) { set_write_state( - t, GRPC_CHTTP2_WRITE_STATE_WRITING_WITH_MORE_AND_COVERED_BY_POLLER); + t, GRPC_CHTTP2_WRITE_STATE_WRITING_WITH_MORE_AND_COVERED_BY_POLLER, + reason); } break; case GRPC_CHTTP2_WRITE_STATE_WRITING_WITH_MORE_AND_COVERED_BY_POLLER: @@ -620,10 +622,10 @@ static void write_action_begin_locked(grpc_exec_ctx *exec_ctx, void *gt, 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(t, GRPC_CHTTP2_WRITE_STATE_WRITING); + set_write_state(t, GRPC_CHTTP2_WRITE_STATE_WRITING, "begin writing"); grpc_exec_ctx_sched(exec_ctx, &t->write_action, GRPC_ERROR_NONE, NULL); } else { - set_write_state(t, GRPC_CHTTP2_WRITE_STATE_IDLE); + set_write_state(t, GRPC_CHTTP2_WRITE_STATE_IDLE, "begin writing nothing"); GRPC_CHTTP2_UNREF_TRANSPORT(exec_ctx, t, "writing"); } GPR_TIMER_END("write_action_begin_locked", 0); @@ -661,11 +663,12 @@ static void write_action_end_locked(grpc_exec_ctx *exec_ctx, void *tp, GPR_UNREACHABLE_CODE(break); case GRPC_CHTTP2_WRITE_STATE_WRITING: GPR_TIMER_MARK("state=writing", 0); - set_write_state(t, GRPC_CHTTP2_WRITE_STATE_IDLE); + set_write_state(t, GRPC_CHTTP2_WRITE_STATE_IDLE, "finish writing"); break; case GRPC_CHTTP2_WRITE_STATE_WRITING_WITH_MORE: GPR_TIMER_MARK("state=writing_stale_no_poller", 0); - set_write_state(t, GRPC_CHTTP2_WRITE_STATE_WRITING); + set_write_state(t, GRPC_CHTTP2_WRITE_STATE_WRITING, + "continue writing [!covered]"); GRPC_CHTTP2_REF_TRANSPORT(t, "writing"); grpc_combiner_execute_finally(exec_ctx, t->combiner, &t->write_action_begin_locked, @@ -673,7 +676,8 @@ static void write_action_end_locked(grpc_exec_ctx *exec_ctx, void *tp, break; case GRPC_CHTTP2_WRITE_STATE_WRITING_WITH_MORE_AND_COVERED_BY_POLLER: GPR_TIMER_MARK("state=writing_stale_with_poller", 0); - set_write_state(t, GRPC_CHTTP2_WRITE_STATE_WRITING); + set_write_state(t, GRPC_CHTTP2_WRITE_STATE_WRITING, + "continue writing [covered]"); GRPC_CHTTP2_REF_TRANSPORT(t, "writing"); grpc_combiner_execute_finally(exec_ctx, t->combiner, &t->write_action_begin_locked, @@ -1887,13 +1891,17 @@ static void incoming_byte_stream_update_flow_control(grpc_exec_ctx *exec_ctx, max_recv_bytes += t->stream_lookahead; if (s->max_recv_bytes < max_recv_bytes) { uint32_t add_max_recv_bytes = max_recv_bytes - s->max_recv_bytes; + bool new_window_write_is_covered_by_poller = + s->max_recv_bytes < have_already; GRPC_CHTTP2_FLOW_CREDIT_STREAM("op", t, s, max_recv_bytes, add_max_recv_bytes); GRPC_CHTTP2_FLOW_CREDIT_STREAM("op", t, s, incoming_window, add_max_recv_bytes); GRPC_CHTTP2_FLOW_CREDIT_STREAM("op", t, s, announce_window, add_max_recv_bytes); - grpc_chttp2_become_writable(exec_ctx, t, s, true, "read_incoming_stream"); + grpc_chttp2_become_writable(exec_ctx, t, s, + new_window_write_is_covered_by_poller, + "read_incoming_stream"); } } diff --git a/src/core/lib/iomgr/combiner.c b/src/core/lib/iomgr/combiner.c index 2b68240a15e..48806abc381 100644 --- a/src/core/lib/iomgr/combiner.c +++ b/src/core/lib/iomgr/combiner.c @@ -142,11 +142,11 @@ static void push_first_on_exec_ctx(grpc_exec_ctx *exec_ctx, void grpc_combiner_execute(grpc_exec_ctx *exec_ctx, grpc_combiner *lock, grpc_closure *cl, grpc_error *error, bool covered_by_poller) { - GRPC_COMBINER_TRACE(gpr_log(GPR_DEBUG, - "C:%p grpc_combiner_execute c=%p cov=%d", lock, - cl, covered_by_poller)); GPR_TIMER_BEGIN("combiner.execute", 0); gpr_atm last = gpr_atm_full_fetch_add(&lock->state, 2); + GRPC_COMBINER_TRACE(gpr_log( + GPR_DEBUG, "C:%p grpc_combiner_execute c=%p cov=%d last=%" PRIdPTR, lock, + cl, covered_by_poller, last)); GPR_ASSERT(last & 1); // ensure lock has not been destroyed cl->error_data.scratch = pack_error_data((error_data){error, covered_by_poller}); @@ -177,6 +177,8 @@ static void offload(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) { static void queue_offload(grpc_exec_ctx *exec_ctx, grpc_combiner *lock) { move_next(exec_ctx); + GRPC_COMBINER_TRACE(gpr_log(GPR_DEBUG, "C:%p queue_offload --> %p", lock, + lock->optional_workqueue)); grpc_workqueue_enqueue(exec_ctx, lock->optional_workqueue, &lock->offload, GRPC_ERROR_NONE); } @@ -189,6 +191,15 @@ bool grpc_combiner_continue_exec_ctx(grpc_exec_ctx *exec_ctx) { return false; } + GRPC_COMBINER_TRACE( + gpr_log(GPR_DEBUG, + "C:%p grpc_combiner_continue_exec_ctx workqueue=%p " + "is_covered_by_poller=%d exec_ctx_ready_to_finish=%d " + "time_to_execute_final_list=%d", + lock, lock->optional_workqueue, is_covered_by_poller(lock), + grpc_exec_ctx_ready_to_finish(exec_ctx), + lock->time_to_execute_final_list)); + if (lock->optional_workqueue != NULL && is_covered_by_poller(lock) && grpc_exec_ctx_ready_to_finish(exec_ctx)) { GPR_TIMER_MARK("offload_from_finished_exec_ctx", 0); @@ -289,9 +300,9 @@ static void enqueue_finally(grpc_exec_ctx *exec_ctx, void *closure, void grpc_combiner_execute_finally(grpc_exec_ctx *exec_ctx, grpc_combiner *lock, grpc_closure *closure, grpc_error *error, bool covered_by_poller) { - GRPC_COMBINER_TRACE(gpr_log(GPR_DEBUG, - "C:%p grpc_combiner_execute_finally c=%p; ac=%p", - lock, closure, exec_ctx->active_combiner)); + GRPC_COMBINER_TRACE(gpr_log( + GPR_DEBUG, "C:%p grpc_combiner_execute_finally c=%p; ac=%p; cov=%d", lock, + closure, exec_ctx->active_combiner, covered_by_poller)); GPR_TIMER_BEGIN("combiner.execute_finally", 0); if (exec_ctx->active_combiner != lock) { GPR_TIMER_MARK("slowpath", 0); diff --git a/src/core/lib/iomgr/iomgr.c b/src/core/lib/iomgr/iomgr.c index d67d388b8c9..4fd83e0b224 100644 --- a/src/core/lib/iomgr/iomgr.c +++ b/src/core/lib/iomgr/iomgr.c @@ -112,6 +112,14 @@ void grpc_iomgr_shutdown(void) { continue; } if (g_root_object.next != &g_root_object) { + if (grpc_iomgr_abort_on_leaks()) { + gpr_log(GPR_DEBUG, "Failed to free %" PRIuPTR + " iomgr objects before shutdown deadline: " + "memory leaks are likely", + count_objects()); + dump_objects("LEAKED"); + abort(); + } gpr_timespec short_deadline = gpr_time_add( gpr_now(GPR_CLOCK_REALTIME), gpr_time_from_millis(100, GPR_TIMESPAN)); if (gpr_cv_wait(&g_rcv, &g_mu, short_deadline)) { @@ -122,9 +130,6 @@ void grpc_iomgr_shutdown(void) { "memory leaks are likely", count_objects()); dump_objects("LEAKED"); - if (grpc_iomgr_abort_on_leaks()) { - abort(); - } } break; } diff --git a/src/core/lib/support/log.c b/src/core/lib/support/log.c index 6fbd947f4b9..af1651dae54 100644 --- a/src/core/lib/support/log.c +++ b/src/core/lib/support/log.c @@ -60,8 +60,9 @@ const char *gpr_log_severity_string(gpr_log_severity severity) { void gpr_log_message(const char *file, int line, gpr_log_severity severity, const char *message) { - if ((gpr_atm)severity < gpr_atm_no_barrier_load(&g_min_severity_to_print)) + if ((gpr_atm)severity < gpr_atm_no_barrier_load(&g_min_severity_to_print)) { return; + } gpr_log_func_args lfargs; memset(&lfargs, 0, sizeof(lfargs)); diff --git a/test/core/end2end/tests/payload.c b/test/core/end2end/tests/payload.c index c37d7fe41aa..ed1c719ef8c 100644 --- a/test/core/end2end/tests/payload.c +++ b/test/core/end2end/tests/payload.c @@ -99,11 +99,11 @@ static void end_test(grpc_end2end_test_fixture *f) { static gpr_slice generate_random_slice() { size_t i; static const char chars[] = "abcdefghijklmnopqrstuvwxyz1234567890"; - char output[1024 * 1024]; /* 1 MB */ - for (i = 0; i < 1024 * 1024 - 1; ++i) { + char output[1024 * 1024]; + for (i = 0; i < GPR_ARRAY_SIZE(output) - 1; ++i) { output[i] = chars[rand() % (int)(sizeof(chars) - 1)]; } - output[1024 * 1024 - 1] = '\0'; + output[GPR_ARRAY_SIZE(output) - 1] = '\0'; return gpr_slice_from_copied_string(output); }