Upmerge branch 'v1.9.x' into 'master'

5 files changed, 168 insertions, 23 deletions

diff --git a/src/core/ext/filters/client_channel/client_channel.cc b/src/core/ext/filters/client_channel/client_channel.cc
index 49522ef3e4..6b93644430 100644
--- a/src/core/ext/filters/client_channel/client_channel.cc
+++ b/src/core/ext/filters/client_channel/client_channel.cc
@@ -1095,6 +1095,7 @@ static void pick_callback_done_locked(void* arg, grpc_error* error) {
             chand, calld);
   }
   async_pick_done_locked(elem, GRPC_ERROR_REF(error));
+  GRPC_CALL_STACK_UNREF(calld->owning_call, "pick_callback");
 }
 
 // Takes a ref to chand->lb_policy and calls grpc_lb_policy_pick_locked().
@@ -1134,6 +1135,7 @@ static bool pick_callback_start_locked(grpc_call_element* elem) {
   GRPC_CLOSURE_INIT(&calld->lb_pick_closure, pick_callback_done_locked, elem,
                     grpc_combiner_scheduler(chand->combiner));
   calld->pick.on_complete = &calld->lb_pick_closure;
+  GRPC_CALL_STACK_REF(calld->owning_call, "pick_callback");
   const bool pick_done =
       grpc_lb_policy_pick_locked(chand->lb_policy, &calld->pick);
   if (pick_done) {
@@ -1142,6 +1144,7 @@ static bool pick_callback_start_locked(grpc_call_element* elem) {
       gpr_log(GPR_DEBUG, "chand=%p calld=%p: pick completed synchronously",
               chand, calld);
     }
+    GRPC_CALL_STACK_UNREF(calld->owning_call, "pick_callback");
   } else {
     GRPC_CALL_STACK_REF(calld->owning_call, "pick_callback_cancel");
     grpc_call_combiner_set_notify_on_cancel(
diff --git a/src/core/ext/filters/max_age/max_age_filter.cc b/src/core/ext/filters/max_age/max_age_filter.cc
index 7b86e4cd6c..a3f9780f3f 100644
--- a/src/core/ext/filters/max_age/max_age_filter.cc
+++ b/src/core/ext/filters/max_age/max_age_filter.cc
@@ -37,6 +37,12 @@
 #define MAX_CONNECTION_IDLE_INTEGER_OPTIONS \
   { DEFAULT_MAX_CONNECTION_IDLE_MS, 1, INT_MAX }
 
+/* States for idle_state in channel_data */
+#define MAX_IDLE_STATE_INIT ((gpr_atm)0)
+#define MAX_IDLE_STATE_SEEN_EXIT_IDLE ((gpr_atm)1)
+#define MAX_IDLE_STATE_SEEN_ENTER_IDLE ((gpr_atm)2)
+#define MAX_IDLE_STATE_TIMER_SET ((gpr_atm)3)
+
 namespace {
 struct channel_data {
   /* We take a reference to the channel stack for the timer callback */
@@ -64,7 +70,7 @@ struct channel_data {
   grpc_millis max_connection_age_grace;
   /* Closure to run when the channel's idle duration reaches max_connection_idle
      and should be closed gracefully */
-  grpc_closure close_max_idle_channel;
+  grpc_closure max_idle_timer_cb;
   /* Closure to run when the channel reaches its max age and should be closed
      gracefully */
   grpc_closure close_max_age_channel;
@@ -85,26 +91,117 @@ struct channel_data {
   grpc_connectivity_state connectivity_state;
   /* Number of active calls */
   gpr_atm call_count;
+  /* TODO(zyc): C++lize this state machine */
+  /* 'idle_state' holds the states of max_idle_timer and channel idleness.
+      It can contain one of the following values:
+     +--------------------------------+----------------+---------+
+     |           idle_state           | max_idle_timer | channel |
+     +--------------------------------+----------------+---------+
+     | MAX_IDLE_STATE_INIT            | unset          | busy    |
+     | MAX_IDLE_STATE_TIMER_SET       | set, valid     | idle    |
+     | MAX_IDLE_STATE_SEEN_EXIT_IDLE  | set, invalid   | busy    |
+     | MAX_IDLE_STATE_SEEN_ENTER_IDLE | set, invalid   | idle    |
+     +--------------------------------+----------------+---------+
+
+     MAX_IDLE_STATE_INIT: The initial and final state of 'idle_state'. The
+     channel has 1 or 1+ active calls, and the the timer is not set. Note that
+     we may put a virtual call to hold this state at channel initialization or
+     shutdown, so that the channel won't enter other states.
+
+     MAX_IDLE_STATE_TIMER_SET: The state after the timer is set and no calls
+     have arrived after the timer is set. The channel must have 0 active call in
+     this state. If the timer is fired in this state, we will close the channel
+     due to idleness.
+
+     MAX_IDLE_STATE_SEEN_EXIT_IDLE: The state after the timer is set and at
+     least one call has arrived after the timer is set. The channel must have 1
+     or 1+ active calls in this state. If the timer is fired in this state, we
+     won't reschudle it.
+
+     MAX_IDLE_STATE_SEEN_ENTER_IDLE: The state after the timer is set and the at
+     least one call has arrived after the timer is set, BUT the channel
+     currently has 1 or 1+ active calls. If the timer is fired in this state, we
+     will reschudle it.
+
+     max_idle_timer will not be cancelled (unless the channel is shutting down).
+     If the timer callback is called when the max_idle_timer is valid (i.e.
+     idle_state is MAX_IDLE_STATE_TIMER_SET), the channel will be closed due to
+     idleness, otherwise the channel won't be changed.
+
+     State transitions:
+         MAX_IDLE_STATE_INIT <-------3------ MAX_IDLE_STATE_SEEN_EXIT_IDLE
+              ^    |                              ^     ^    |
+              |    |                              |     |    |
+              1    2     +-----------4------------+     6    7
+              |    |     |                              |    |
+              |    v     |                              |    v
+       MAX_IDLE_STATE_TIMER_SET <----5------ MAX_IDLE_STATE_SEEN_ENTER_IDLE
+
+     For 1, 3, 5 :  See max_idle_timer_cb() function
+     For 2, 7    :  See decrease_call_count() function
+     For 4, 6    :  See increase_call_count() function */
+  gpr_atm idle_state;
+  /* Time when the channel finished its last outstanding call, in grpc_millis */
+  gpr_atm last_enter_idle_time_millis;
 };
 }  // namespace
 
 /* Increase the nubmer of active calls. Before the increasement, if there are no
    calls, the max_idle_timer should be cancelled. */
 static void increase_call_count(channel_data* chand) {
+  /* Exit idle */
   if (gpr_atm_full_fetch_add(&chand->call_count, 1) == 0) {
-    grpc_timer_cancel(&chand->max_idle_timer);
+    while (true) {
+      gpr_atm idle_state = gpr_atm_acq_load(&chand->idle_state);
+      switch (idle_state) {
+        case MAX_IDLE_STATE_TIMER_SET:
+          /* max_idle_timer_cb may have already set idle_state to
+             MAX_IDLE_STATE_INIT, in this case, we don't need to set it to
+             MAX_IDLE_STATE_SEEN_EXIT_IDLE */
+          gpr_atm_rel_cas(&chand->idle_state, MAX_IDLE_STATE_TIMER_SET,
+                          MAX_IDLE_STATE_SEEN_EXIT_IDLE);
+          return;
+        case MAX_IDLE_STATE_SEEN_ENTER_IDLE:
+          gpr_atm_rel_store(&chand->idle_state, MAX_IDLE_STATE_SEEN_EXIT_IDLE);
+          return;
+        default:
+          /* try again */
+          break;
+      }
+    }
   }
 }
 
 /* Decrease the nubmer of active calls. After the decrement, if there are no
    calls, the max_idle_timer should be started. */
 static void decrease_call_count(channel_data* chand) {
+  /* Enter idle */
   if (gpr_atm_full_fetch_add(&chand->call_count, -1) == 1) {
-    GRPC_CHANNEL_STACK_REF(chand->channel_stack, "max_age max_idle_timer");
-    grpc_timer_init(
-        &chand->max_idle_timer,
-        grpc_core::ExecCtx::Get()->Now() + chand->max_connection_idle,
-        &chand->close_max_idle_channel);
+    gpr_atm_no_barrier_store(&chand->last_enter_idle_time_millis,
+                             (gpr_atm)grpc_core::ExecCtx::Get()->Now());
+    while (true) {
+      gpr_atm idle_state = gpr_atm_acq_load(&chand->idle_state);
+      switch (idle_state) {
+        case MAX_IDLE_STATE_INIT:
+          GRPC_CHANNEL_STACK_REF(chand->channel_stack,
+                                 "max_age max_idle_timer");
+          grpc_timer_init(
+              &chand->max_idle_timer,
+              grpc_core::ExecCtx::Get()->Now() + chand->max_connection_idle,
+              &chand->max_idle_timer_cb);
+          gpr_atm_rel_store(&chand->idle_state, MAX_IDLE_STATE_TIMER_SET);
+          return;
+        case MAX_IDLE_STATE_SEEN_EXIT_IDLE:
+          if (gpr_atm_rel_cas(&chand->idle_state, MAX_IDLE_STATE_SEEN_EXIT_IDLE,
+                              MAX_IDLE_STATE_SEEN_ENTER_IDLE)) {
+            return;
+          }
+          break;
+        default:
+          /* try again */
+          break;
+      }
+    }
   }
 }
 
@@ -152,20 +249,58 @@ static void start_max_age_grace_timer_after_goaway_op(void* arg,
                            "max_age start_max_age_grace_timer_after_goaway_op");
 }
 
-static void close_max_idle_channel(void* arg, grpc_error* error) {
+static void close_max_idle_channel(channel_data* chand) {
+  /* Prevent the max idle timer from being set again */
+  gpr_atm_no_barrier_fetch_add(&chand->call_count, 1);
+  grpc_transport_op* op = grpc_make_transport_op(nullptr);
+  op->goaway_error =
+      grpc_error_set_int(GRPC_ERROR_CREATE_FROM_STATIC_STRING("max_idle"),
+                         GRPC_ERROR_INT_HTTP2_ERROR, GRPC_HTTP2_NO_ERROR);
+  grpc_channel_element* elem =
+      grpc_channel_stack_element(chand->channel_stack, 0);
+  elem->filter->start_transport_op(elem, op);
+}
+
+static void max_idle_timer_cb(void* arg, grpc_error* error) {
   channel_data* chand = (channel_data*)arg;
   if (error == GRPC_ERROR_NONE) {
-    /* Prevent the max idle timer from being set again */
-    gpr_atm_no_barrier_fetch_add(&chand->call_count, 1);
-    grpc_transport_op* op = grpc_make_transport_op(nullptr);
-    op->goaway_error =
-        grpc_error_set_int(GRPC_ERROR_CREATE_FROM_STATIC_STRING("max_idle"),
-                           GRPC_ERROR_INT_HTTP2_ERROR, GRPC_HTTP2_NO_ERROR);
-    grpc_channel_element* elem =
-        grpc_channel_stack_element(chand->channel_stack, 0);
-    elem->filter->start_transport_op(elem, op);
-  } else if (error != GRPC_ERROR_CANCELLED) {
-    GRPC_LOG_IF_ERROR("close_max_idle_channel", error);
+    bool try_again = true;
+    while (try_again) {
+      gpr_atm idle_state = gpr_atm_acq_load(&chand->idle_state);
+      switch (idle_state) {
+        case MAX_IDLE_STATE_TIMER_SET:
+          close_max_idle_channel(chand);
+          /* This MAX_IDLE_STATE_INIT is a final state, we don't have to check
+           * if idle_state has been changed */
+          gpr_atm_rel_store(&chand->idle_state, MAX_IDLE_STATE_INIT);
+          try_again = false;
+          break;
+        case MAX_IDLE_STATE_SEEN_EXIT_IDLE:
+          if (gpr_atm_rel_cas(&chand->idle_state, MAX_IDLE_STATE_SEEN_EXIT_IDLE,
+                              MAX_IDLE_STATE_INIT)) {
+            try_again = false;
+          }
+          break;
+        case MAX_IDLE_STATE_SEEN_ENTER_IDLE:
+          GRPC_CHANNEL_STACK_REF(chand->channel_stack,
+                                 "max_age max_idle_timer");
+          grpc_timer_init(&chand->max_idle_timer,
+                          (grpc_millis)gpr_atm_no_barrier_load(
+                              &chand->last_enter_idle_time_millis) +
+                              chand->max_connection_idle,
+                          &chand->max_idle_timer_cb);
+          /* idle_state may have already been set to
+             MAX_IDLE_STATE_SEEN_EXIT_IDLE by increase_call_count(), in this
+             case, we don't need to set it to MAX_IDLE_STATE_TIMER_SET */
+          gpr_atm_rel_cas(&chand->idle_state, MAX_IDLE_STATE_SEEN_ENTER_IDLE,
+                          MAX_IDLE_STATE_TIMER_SET);
+          try_again = false;
+          break;
+        default:
+          /* try again */
+          break;
+      }
+    }
   }
   GRPC_CHANNEL_STACK_UNREF(chand->channel_stack, "max_age max_idle_timer");
 }
@@ -288,6 +423,9 @@ static grpc_error* init_channel_elem(grpc_channel_element* elem,
   chand->max_connection_idle = DEFAULT_MAX_CONNECTION_IDLE_MS == INT_MAX
                                    ? GRPC_MILLIS_INF_FUTURE
                                    : DEFAULT_MAX_CONNECTION_IDLE_MS;
+  chand->idle_state = MAX_IDLE_STATE_INIT;
+  gpr_atm_no_barrier_store(&chand->last_enter_idle_time_millis,
+                           GRPC_MILLIS_INF_PAST);
   for (size_t i = 0; i < args->channel_args->num_args; ++i) {
     if (0 == strcmp(args->channel_args->args[i].key,
                     GRPC_ARG_MAX_CONNECTION_AGE_MS)) {
@@ -311,8 +449,8 @@ static grpc_error* init_channel_elem(grpc_channel_element* elem,
           value == INT_MAX ? GRPC_MILLIS_INF_FUTURE : value;
     }
   }
-  GRPC_CLOSURE_INIT(&chand->close_max_idle_channel, close_max_idle_channel,
-                    chand, grpc_schedule_on_exec_ctx);
+  GRPC_CLOSURE_INIT(&chand->max_idle_timer_cb, max_idle_timer_cb, chand,
+                    grpc_schedule_on_exec_ctx);
   GRPC_CLOSURE_INIT(&chand->close_max_age_channel, close_max_age_channel, chand,
                     grpc_schedule_on_exec_ctx);
   GRPC_CLOSURE_INIT(&chand->force_close_max_age_channel,
diff --git a/src/core/lib/security/transport/client_auth_filter.cc b/src/core/lib/security/transport/client_auth_filter.cc
index 16814d2598..802503c868 100644
--- a/src/core/lib/security/transport/client_auth_filter.cc
+++ b/src/core/lib/security/transport/client_auth_filter.cc
@@ -118,6 +118,7 @@ static void on_credentials_metadata(void* arg, grpc_error* input_error) {
     grpc_transport_stream_op_batch_finish_with_failure(batch, error,
                                                        calld->call_combiner);
   }
+  GRPC_CALL_STACK_UNREF(calld->owning_call, "get_request_metadata");
 }
 
 void grpc_auth_metadata_context_build(
@@ -208,7 +209,7 @@ static void send_security_metadata(grpc_call_element* elem,
       chand->auth_context, &calld->auth_md_context);
 
   GPR_ASSERT(calld->pollent != nullptr);
-
+  GRPC_CALL_STACK_REF(calld->owning_call, "get_request_metadata");
   GRPC_CLOSURE_INIT(&calld->async_result_closure, on_credentials_metadata,
                     batch, grpc_schedule_on_exec_ctx);
   grpc_error* error = GRPC_ERROR_NONE;
@@ -250,6 +251,7 @@ static void on_host_checked(void* arg, grpc_error* error) {
         calld->call_combiner);
     gpr_free(error_msg);
   }
+  GRPC_CALL_STACK_UNREF(calld->owning_call, "check_call_host");
 }
 
 static void cancel_check_call_host(void* arg, grpc_error* error) {
@@ -312,6 +314,7 @@ static void auth_start_transport_stream_op_batch(
     }
     if (calld->have_host) {
       batch->handler_private.extra_arg = elem;
+      GRPC_CALL_STACK_REF(calld->owning_call, "check_call_host");
       GRPC_CLOSURE_INIT(&calld->async_result_closure, on_host_checked, batch,
                         grpc_schedule_on_exec_ctx);
       char* call_host = grpc_slice_to_c_string(calld->host);
diff --git a/src/core/lib/surface/call.cc b/src/core/lib/surface/call.cc
index f2096d8937..b538cc0212 100644
--- a/src/core/lib/surface/call.cc
+++ b/src/core/lib/surface/call.cc
@@ -1032,6 +1032,7 @@ static grpc_stream_compression_algorithm decode_stream_compression(
 static void publish_app_metadata(grpc_call* call, grpc_metadata_batch* b,
                                  int is_trailing) {
   if (b->list.count == 0) return;
+  if (is_trailing && call->buffered_metadata[1] == nullptr) return;
   GPR_TIMER_SCOPE("publish_app_metadata", 0);
   grpc_metadata_array* dest;
   grpc_metadata* mdusr;
diff --git a/src/core/lib/surface/version.cc b/src/core/lib/surface/version.cc
index 153b6e0297..51daad0368 100644
--- a/src/core/lib/surface/version.cc
+++ b/src/core/lib/surface/version.cc
@@ -23,4 +23,4 @@
 
 const char* grpc_version_string(void) { return "6.0.0-dev"; }
 
-const char* grpc_g_stands_for(void) { return "glossy"; }
+const char* grpc_g_stands_for(void) { return "glamorous"; }