From 1784e6c96258f41e6dce6c00bc5120ab4915d6bb Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Tue, 6 Apr 2021 23:34:33 -0700 Subject: [PATCH] Revert #16288 (#25827) --- .../filters/client_channel/health/health_check_client.cc | 5 +---- src/core/lib/iomgr/call_combiner.h | 4 ---- src/core/lib/security/transport/client_auth_filter.cc | 8 ++++++++ src/core/lib/security/transport/server_auth_filter.cc | 4 ++++ src/core/lib/surface/call.cc | 5 +---- 5 files changed, 14 insertions(+), 12 deletions(-) diff --git a/src/core/ext/filters/client_channel/health/health_check_client.cc b/src/core/ext/filters/client_channel/health/health_check_client.cc index 7885fadd2c3..ab720231f4f 100644 --- a/src/core/ext/filters/client_channel/health/health_check_client.cc +++ b/src/core/ext/filters/client_channel/health/health_check_client.cc @@ -269,11 +269,8 @@ HealthCheckClient::CallState::~CallState() { // Unset the call combiner cancellation closure. This has the // effect of scheduling the previously set cancellation closure, if // any, so that it can release any internal references it may be - // holding to the call stack. Also flush the closures on exec_ctx so that - // filters that schedule cancel notification closures on exec_ctx do not - // need to take a ref of the call stack to guarantee closure liveness. + // holding to the call stack. call_combiner_.SetNotifyOnCancel(nullptr); - ExecCtx::Get()->Flush(); arena_->Destroy(); } diff --git a/src/core/lib/iomgr/call_combiner.h b/src/core/lib/iomgr/call_combiner.h index 44acd9fe870..5d2b00cac13 100644 --- a/src/core/lib/iomgr/call_combiner.h +++ b/src/core/lib/iomgr/call_combiner.h @@ -94,10 +94,6 @@ class CallCombiner { /// cancellation; this effectively unregisters the previously set closure. /// However, most filters will not need to explicitly unregister their /// callbacks, as this is done automatically when the call is destroyed. - /// Filters that schedule the cancellation closure on ExecCtx do not need - /// to take a ref on the call stack to guarantee closure liveness. This is - /// done by explicitly flushing ExecCtx after the unregistration during - /// call destruction. void SetNotifyOnCancel(grpc_closure* closure); /// Indicates that the call has been cancelled. diff --git a/src/core/lib/security/transport/client_auth_filter.cc b/src/core/lib/security/transport/client_auth_filter.cc index c155be81230..1e6b89e8804 100644 --- a/src/core/lib/security/transport/client_auth_filter.cc +++ b/src/core/lib/security/transport/client_auth_filter.cc @@ -232,6 +232,7 @@ static void cancel_get_request_metadata(void* arg, grpc_error* error) { calld->creds->cancel_get_request_metadata(&calld->md_array, GRPC_ERROR_REF(error)); } + GRPC_CALL_STACK_UNREF(calld->owning_call, "cancel_get_request_metadata"); } static void send_security_metadata(grpc_call_element* elem, @@ -320,6 +321,9 @@ static void send_security_metadata(grpc_call_element* elem, GRPC_ERROR_UNREF(error); } else { // Async return; register cancellation closure with call combiner. + // TODO(yashykt): We would not need this ref if call combiners used + // Closure::Run() instead of ExecCtx::Run() + GRPC_CALL_STACK_REF(calld->owning_call, "cancel_get_request_metadata"); calld->call_combiner->SetNotifyOnCancel(GRPC_CLOSURE_INIT( &calld->get_request_metadata_cancel_closure, cancel_get_request_metadata, elem, grpc_schedule_on_exec_ctx)); @@ -356,6 +360,7 @@ static void cancel_check_call_host(void* arg, grpc_error* error) { chand->security_connector->cancel_check_call_host( &calld->async_result_closure, GRPC_ERROR_REF(error)); } + GRPC_CALL_STACK_UNREF(calld->owning_call, "cancel_check_call_host"); } static void client_auth_start_transport_stream_op_batch( @@ -390,6 +395,9 @@ static void client_auth_start_transport_stream_op_batch( GRPC_ERROR_UNREF(error); } else { // Async return; register cancellation closure with call combiner. + // TODO(yashykt): We would not need this ref if call combiners used + // Closure::Run() instead of ExecCtx::Run() + GRPC_CALL_STACK_REF(calld->owning_call, "cancel_check_call_host"); calld->call_combiner->SetNotifyOnCancel(GRPC_CLOSURE_INIT( &calld->check_call_host_cancel_closure, cancel_check_call_host, elem, grpc_schedule_on_exec_ctx)); diff --git a/src/core/lib/security/transport/server_auth_filter.cc b/src/core/lib/security/transport/server_auth_filter.cc index 925a70a24c7..d4ec09a0c10 100644 --- a/src/core/lib/security/transport/server_auth_filter.cc +++ b/src/core/lib/security/transport/server_auth_filter.cc @@ -206,6 +206,7 @@ static void cancel_call(void* arg, grpc_error* error) { on_md_processing_done_inner(elem, nullptr, 0, nullptr, 0, GRPC_ERROR_REF(error)); } + GRPC_CALL_STACK_UNREF(calld->owning_call, "cancel_call"); } static void recv_initial_metadata_ready(void* arg, grpc_error* error) { @@ -218,6 +219,9 @@ static void recv_initial_metadata_ready(void* arg, grpc_error* error) { chand->creds->auth_metadata_processor().process != nullptr) { // We're calling out to the application, so we need to make sure // to drop the call combiner early if we get cancelled. + // TODO(yashykt): We would not need this ref if call combiners used + // Closure::Run() instead of ExecCtx::Run() + GRPC_CALL_STACK_REF(calld->owning_call, "cancel_call"); GRPC_CLOSURE_INIT(&calld->cancel_closure, cancel_call, elem, grpc_schedule_on_exec_ctx); calld->call_combiner->SetNotifyOnCancel(&calld->cancel_closure); diff --git a/src/core/lib/surface/call.cc b/src/core/lib/surface/call.cc index d749d396eda..33ae8e16d16 100644 --- a/src/core/lib/surface/call.cc +++ b/src/core/lib/surface/call.cc @@ -610,11 +610,8 @@ void grpc_call_unref(grpc_call* c) { // Unset the call combiner cancellation closure. This has the // effect of scheduling the previously set cancellation closure, if // any, so that it can release any internal references it may be - // holding to the call stack. Also flush the closures on exec_ctx so that - // filters that schedule cancel notification closures on exec_ctx do not - // need to take a ref of the call stack to guarantee closure liveness. + // holding to the call stack. c->call_combiner.SetNotifyOnCancel(nullptr); - grpc_core::ExecCtx::Get()->Flush(); } GRPC_CALL_INTERNAL_UNREF(c, "destroy"); }