From c9e1a71c8e104a575135a8a4b1a4dc7d4aee1234 Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Fri, 22 Mar 2019 16:05:04 -0700 Subject: [PATCH 1/3] Remove unnecessary hack which causes data races --- .../chttp2/transport/chttp2_transport.cc | 24 +++++++------------ 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc index 49ec869d707..0d25645c07e 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc @@ -1409,21 +1409,13 @@ static void perform_stream_op_locked(void* stream_op, } grpc_closure* on_complete = op->on_complete; - // TODO(roth): This is a hack needed because we use data inside of the - // closure itself to do the barrier calculation (i.e., to ensure that - // we don't schedule the closure until all ops in the batch have been - // completed). This can go away once we move to a new C++ closure API - // that provides the ability to create a barrier closure. - if (on_complete == nullptr) { - on_complete = GRPC_CLOSURE_INIT(&op->handler_private.closure, do_nothing, - nullptr, grpc_schedule_on_exec_ctx); + if(on_complete != nullptr) { + /* This batch has send ops. Use final_data as a barrier until enqueue time; + * the inital counter is dropped at the end of this function */ + on_complete->next_data.scratch = CLOSURE_BARRIER_FIRST_REF_BIT; + on_complete->error_data.error = GRPC_ERROR_NONE; } - /* use final_data as a barrier until enqueue time; the inital counter is - dropped at the end of this function */ - on_complete->next_data.scratch = CLOSURE_BARRIER_FIRST_REF_BIT; - on_complete->error_data.error = GRPC_ERROR_NONE; - if (op->cancel_stream) { GRPC_STATS_INC_HTTP2_OP_CANCEL(); grpc_chttp2_cancel_stream(t, s, op_payload->cancel_stream.cancel_error); @@ -1672,8 +1664,10 @@ static void perform_stream_op_locked(void* stream_op, grpc_chttp2_maybe_complete_recv_trailing_metadata(t, s); } - grpc_chttp2_complete_closure_step(t, s, &on_complete, GRPC_ERROR_NONE, - "op->on_complete"); + if(on_complete != nullptr) { + grpc_chttp2_complete_closure_step(t, s, &on_complete, GRPC_ERROR_NONE, + "op->on_complete"); + } GRPC_CHTTP2_STREAM_UNREF(s, "perform_stream_op"); } From eab66cb6cc1afd4586a40e16667a59234a133490 Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Fri, 22 Mar 2019 16:08:11 -0700 Subject: [PATCH 2/3] Remove unused function --- src/core/ext/transport/chttp2/transport/chttp2_transport.cc | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc index 0d25645c07e..d334b470bf7 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc @@ -1363,8 +1363,6 @@ static void complete_fetch_locked(void* gs, grpc_error* error) { } } -static void do_nothing(void* arg, grpc_error* error) {} - static void log_metadata(const grpc_metadata_batch* md_batch, uint32_t id, bool is_client, bool is_initial) { for (grpc_linked_mdelem* md = md_batch->list.head; md != nullptr; @@ -1409,7 +1407,7 @@ static void perform_stream_op_locked(void* stream_op, } grpc_closure* on_complete = op->on_complete; - if(on_complete != nullptr) { + if (on_complete != nullptr) { /* This batch has send ops. Use final_data as a barrier until enqueue time; * the inital counter is dropped at the end of this function */ on_complete->next_data.scratch = CLOSURE_BARRIER_FIRST_REF_BIT; @@ -1664,7 +1662,7 @@ static void perform_stream_op_locked(void* stream_op, grpc_chttp2_maybe_complete_recv_trailing_metadata(t, s); } - if(on_complete != nullptr) { + if (on_complete != nullptr) { grpc_chttp2_complete_closure_step(t, s, &on_complete, GRPC_ERROR_NONE, "op->on_complete"); } From 072749b38c7008943b66eb4cd9bfa420636f0df9 Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Fri, 22 Mar 2019 16:58:14 -0700 Subject: [PATCH 3/3] Reviewer comments --- src/core/ext/transport/chttp2/transport/chttp2_transport.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc index d334b470bf7..6e2d323fe6c 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc @@ -1407,9 +1407,10 @@ static void perform_stream_op_locked(void* stream_op, } grpc_closure* on_complete = op->on_complete; + // on_complete will be null if and only if there are no send ops in the batch. if (on_complete != nullptr) { - /* This batch has send ops. Use final_data as a barrier until enqueue time; - * the inital counter is dropped at the end of this function */ + // This batch has send ops. Use final_data as a barrier until enqueue time; + // the inital counter is dropped at the end of this function. on_complete->next_data.scratch = CLOSURE_BARRIER_FIRST_REF_BIT; on_complete->error_data.error = GRPC_ERROR_NONE; }