diff options
Diffstat (limited to 'src')
| -rw-r--r-- | src/core/ext/filters/client_channel/client_channel.c | 10 | ||||
| -rw-r--r-- | src/core/lib/iomgr/call_combiner.h | 26 | ||||
| -rw-r--r-- | src/core/lib/security/transport/client_auth_filter.c | 10 | ||||
| -rw-r--r-- | src/core/lib/security/transport/server_auth_filter.c | 1 | ||||
| -rw-r--r-- | src/core/lib/surface/call.c | 6 |
5 files changed, 26 insertions, 27 deletions
diff --git a/src/core/ext/filters/client_channel/client_channel.c b/src/core/ext/filters/client_channel/client_channel.c index 8b0d2ee61f..c24e52ec1d 100644 --- a/src/core/ext/filters/client_channel/client_channel.c +++ b/src/core/ext/filters/client_channel/client_channel.c @@ -1083,10 +1083,9 @@ static void pick_after_resolver_result_cancel_locked(grpc_exec_ctx *exec_ctx, // it's safe to call subchannel_ready_locked() here -- we are // essentially calling it here instead of calling it in // pick_after_resolver_result_done_locked(). - subchannel_ready_locked( - exec_ctx, elem, - GRPC_ERROR_CREATE_REFERENCING_FROM_STATIC_STRING("Pick cancelled", - &error, 1)); + subchannel_ready_locked(exec_ctx, elem, + GRPC_ERROR_CREATE_REFERENCING_FROM_STATIC_STRING( + "Pick cancelled", &error, 1)); } static void pick_after_resolver_result_done_locked(grpc_exec_ctx *exec_ctx, @@ -1105,8 +1104,6 @@ static void pick_after_resolver_result_done_locked(grpc_exec_ctx *exec_ctx, grpc_call_element *elem = args->elem; channel_data *chand = elem->channel_data; call_data *calld = elem->call_data; - grpc_call_combiner_set_notify_on_cancel(exec_ctx, calld->call_combiner, - NULL); if (error != GRPC_ERROR_NONE) { if (GRPC_TRACER_ON(grpc_client_channel_trace)) { gpr_log(GPR_DEBUG, "chand=%p calld=%p: resolver failed to return data", @@ -1177,7 +1174,6 @@ static void pick_callback_done_locked(grpc_exec_ctx *exec_ctx, void *arg, gpr_log(GPR_DEBUG, "chand=%p calld=%p: pick completed asynchronously", chand, calld); } - grpc_call_combiner_set_notify_on_cancel(exec_ctx, calld->call_combiner, NULL); GPR_ASSERT(calld->lb_policy != NULL); GRPC_LB_POLICY_UNREF(exec_ctx, calld->lb_policy, "pick_subchannel"); calld->lb_policy = NULL; diff --git a/src/core/lib/iomgr/call_combiner.h b/src/core/lib/iomgr/call_combiner.h index 11802b6eaa..5cfb3f0c07 100644 --- a/src/core/lib/iomgr/call_combiner.h +++ b/src/core/lib/iomgr/call_combiner.h @@ -87,20 +87,28 @@ void grpc_call_combiner_stop(grpc_exec_ctx* exec_ctx, const char* reason); #endif -/// Tells \a call_combiner to schedule \a closure when +/// Registers \a closure to be invoked by \a call_combiner when /// grpc_call_combiner_cancel() is called. /// -/// If grpc_call_combiner_cancel() was previously called, \a closure will be -/// scheduled immediately. +/// Once a closure is registered, it will always be scheduled exactly +/// once; this allows the closure to hold references that will be freed +/// regardless of whether or not the call was cancelled. If a cancellation +/// does occur, the closure will be scheduled with the cancellation error; +/// otherwise, it will be scheduled with GRPC_ERROR_NONE. +/// +/// The closure will be scheduled in the following cases: +/// - If grpc_call_combiner_cancel() was called prior to registering the +/// closure, it will be scheduled immediately with the cancelation error. +/// - If grpc_call_combiner_cancel() is called after registering the +/// closure, the closure will be scheduled with the cancellation error. +/// - If grpc_call_combiner_set_notify_on_cancel() is called again to +/// register a new cancellation closure, the previous cancellation +/// closure will be scheduled with GRPC_ERROR_NONE. /// /// If \a closure is NULL, then no closure will be invoked on /// cancellation; this effectively unregisters the previously set closure. -/// -/// If a closure was set via a previous call to -/// grpc_call_combiner_set_notify_on_cancel(), the previous closure will be -/// scheduled immediately with GRPC_ERROR_NONE. This ensures that -/// \a closure will be scheduled exactly once, which allows callers to clean -/// up resources they may be holding for the callback. +/// However, most filters will not need to explicitly unregister their +/// callbacks, as this is done automatically when the call is destroyed. void grpc_call_combiner_set_notify_on_cancel(grpc_exec_ctx* exec_ctx, grpc_call_combiner* call_combiner, grpc_closure* closure); diff --git a/src/core/lib/security/transport/client_auth_filter.c b/src/core/lib/security/transport/client_auth_filter.c index 69eb612eff..dd7dd44e79 100644 --- a/src/core/lib/security/transport/client_auth_filter.c +++ b/src/core/lib/security/transport/client_auth_filter.c @@ -95,7 +95,6 @@ static void on_credentials_metadata(grpc_exec_ctx *exec_ctx, void *arg, grpc_transport_stream_op_batch *batch = (grpc_transport_stream_op_batch *)arg; grpc_call_element *elem = batch->handler_private.extra_arg; call_data *calld = elem->call_data; - grpc_call_combiner_set_notify_on_cancel(exec_ctx, calld->call_combiner, NULL); reset_auth_metadata_context(&calld->auth_md_context); grpc_error *error = GRPC_ERROR_REF(input_error); if (error == GRPC_ERROR_NONE) { @@ -228,7 +227,6 @@ static void on_host_checked(grpc_exec_ctx *exec_ctx, void *arg, grpc_transport_stream_op_batch *batch = (grpc_transport_stream_op_batch *)arg; grpc_call_element *elem = batch->handler_private.extra_arg; call_data *calld = elem->call_data; - grpc_call_combiner_set_notify_on_cancel(exec_ctx, calld->call_combiner, NULL); if (error == GRPC_ERROR_NONE) { send_security_metadata(exec_ctx, elem, batch); } else { @@ -318,14 +316,6 @@ static void auth_start_transport_stream_op_batch( on_host_checked(exec_ctx, batch, error); GRPC_ERROR_UNREF(error); } else { -// FIXME: if grpc_channel_security_connector_check_call_host() invokes -// the callback in this thread before returning, then we'll call -// grpc_call_combiner_set_notify_on_cancel() to set it "back" to NULL -// *before* we call this to set it to the cancel function. -// Can't just do this before calling -// grpc_channel_security_connector_check_call_host(), because then the -// cancellation might be invoked before we actually send the request. -// May need to fix the credentials plugin API to deal with this. // Async return; register cancellation closure with call combiner. GRPC_CALL_STACK_REF(calld->owning_call, "cancel_check_call_host"); grpc_call_combiner_set_notify_on_cancel( diff --git a/src/core/lib/security/transport/server_auth_filter.c b/src/core/lib/security/transport/server_auth_filter.c index 6cf61c03a2..7f523c0883 100644 --- a/src/core/lib/security/transport/server_auth_filter.c +++ b/src/core/lib/security/transport/server_auth_filter.c @@ -97,7 +97,6 @@ static void on_md_processing_done_inner(grpc_exec_ctx *exec_ctx, grpc_error *error) { call_data *calld = elem->call_data; grpc_transport_stream_op_batch *batch = calld->recv_initial_metadata_batch; - grpc_call_combiner_set_notify_on_cancel(exec_ctx, calld->call_combiner, NULL); /* TODO(jboeuf): Implement support for response_md. */ if (response_md != NULL && num_response_md > 0) { gpr_log(GPR_INFO, diff --git a/src/core/lib/surface/call.c b/src/core/lib/surface/call.c index 03eaaf99ac..3aa20ffcd7 100644 --- a/src/core/lib/surface/call.c +++ b/src/core/lib/surface/call.c @@ -598,6 +598,12 @@ void grpc_call_unref(grpc_call *c) { if (cancel) { cancel_with_error(&exec_ctx, c, STATUS_FROM_API_OVERRIDE, GRPC_ERROR_CANCELLED); + } else { + // 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. + grpc_call_combiner_set_notify_on_cancel(&exec_ctx, &c->call_combiner, NULL); } GRPC_CALL_INTERNAL_UNREF(&exec_ctx, c, "destroy"); grpc_exec_ctx_finish(&exec_ctx); |