Merge pull request #14281 from AspirinSJL/fake_resolver_results_upon_error

Add a results_upon_error setter to fake resolver

2 files changed, 64 insertions, 20 deletions

diff --git a/src/core/ext/filters/client_channel/resolver/fake/fake_resolver.cc b/src/core/ext/filters/client_channel/resolver/fake/fake_resolver.cc
index eaa5e6ac49..e945d08202 100644
--- a/src/core/ext/filters/client_channel/resolver/fake/fake_resolver.cc
+++ b/src/core/ext/filters/client_channel/resolver/fake/fake_resolver.cc
@@ -47,10 +47,10 @@
 //
 
 typedef struct {
-  // base class -- must be first
+  // Base class -- must be first
   grpc_resolver base;
 
-  // passed-in parameters
+  // Passed-in parameters
   grpc_channel_args* channel_args;
 
   // If not NULL, the next set of resolution results to be returned to
@@ -61,9 +61,16 @@ typedef struct {
   // fake_resolver_channel_saw_error_locked().
   grpc_channel_args* results_upon_error;
 
-  // pending next completion, or NULL
+  // TODO(juanlishen): This can go away once pick_first is changed to not throw
+  // away its subchannels, since that will eliminate its dependence on
+  // channel_saw_error_locked() causing an immediate resolver return.
+  // A copy of the most-recently used resolution results.
+  grpc_channel_args* last_used_results;
+
+  // Pending next completion, or NULL
   grpc_closure* next_completion;
-  // target result address for next completion
+
+  // Target result address for next completion
   grpc_channel_args** target_result;
 } fake_resolver;
 
@@ -71,6 +78,7 @@ static void fake_resolver_destroy(grpc_resolver* gr) {
   fake_resolver* r = (fake_resolver*)gr;
   grpc_channel_args_destroy(r->next_results);
   grpc_channel_args_destroy(r->results_upon_error);
+  grpc_channel_args_destroy(r->last_used_results);
   grpc_channel_args_destroy(r->channel_args);
   gpr_free(r);
 }
@@ -98,9 +106,15 @@ static void fake_resolver_maybe_finish_next_locked(fake_resolver* r) {
 
 static void fake_resolver_channel_saw_error_locked(grpc_resolver* resolver) {
   fake_resolver* r = (fake_resolver*)resolver;
-  if (r->next_results == nullptr && r->results_upon_error != nullptr) {
-    // Pretend we re-resolved.
+  // A resolution must have been returned before an error is seen.
+  GPR_ASSERT(r->last_used_results != nullptr);
+  grpc_channel_args_destroy(r->next_results);
+  if (r->results_upon_error != nullptr) {
     r->next_results = grpc_channel_args_copy(r->results_upon_error);
+  } else {
+    // If results_upon_error is unavailable, re-resolve with the most-recently
+    // used results to avoid a no-op re-resolution.
+    r->next_results = grpc_channel_args_copy(r->last_used_results);
   }
   fake_resolver_maybe_finish_next_locked(r);
 }
@@ -149,35 +163,56 @@ void grpc_fake_resolver_response_generator_unref(
 typedef struct set_response_closure_arg {
   grpc_closure set_response_closure;
   grpc_fake_resolver_response_generator* generator;
-  grpc_channel_args* next_response;
+  grpc_channel_args* response;
+  bool upon_error;
 } set_response_closure_arg;
 
-static void set_response_closure_fn(void* arg, grpc_error* error) {
+static void set_response_closure_locked(void* arg, grpc_error* error) {
   set_response_closure_arg* closure_arg = (set_response_closure_arg*)arg;
   grpc_fake_resolver_response_generator* generator = closure_arg->generator;
   fake_resolver* r = generator->resolver;
-  if (r->next_results != nullptr) {
+  if (!closure_arg->upon_error) {
     grpc_channel_args_destroy(r->next_results);
-  }
-  r->next_results = closure_arg->next_response;
-  if (r->results_upon_error != nullptr) {
+    r->next_results = closure_arg->response;
+    grpc_channel_args_destroy(r->last_used_results);
+    r->last_used_results = grpc_channel_args_copy(closure_arg->response);
+    fake_resolver_maybe_finish_next_locked(r);
+  } else {
     grpc_channel_args_destroy(r->results_upon_error);
+    r->results_upon_error = closure_arg->response;
   }
-  r->results_upon_error = grpc_channel_args_copy(closure_arg->next_response);
   gpr_free(closure_arg);
-  fake_resolver_maybe_finish_next_locked(r);
 }
 
 void grpc_fake_resolver_response_generator_set_response(
     grpc_fake_resolver_response_generator* generator,
-    grpc_channel_args* next_response) {
+    grpc_channel_args* response) {
+  GPR_ASSERT(generator->resolver != nullptr);
+  GPR_ASSERT(response != nullptr);
+  set_response_closure_arg* closure_arg =
+      (set_response_closure_arg*)gpr_zalloc(sizeof(*closure_arg));
+  closure_arg->generator = generator;
+  closure_arg->response = grpc_channel_args_copy(response);
+  closure_arg->upon_error = false;
+  GRPC_CLOSURE_SCHED(GRPC_CLOSURE_INIT(&closure_arg->set_response_closure,
+                                       set_response_closure_locked, closure_arg,
+                                       grpc_combiner_scheduler(
+                                           generator->resolver->base.combiner)),
+                     GRPC_ERROR_NONE);
+}
+
+void grpc_fake_resolver_response_generator_set_response_upon_error(
+    grpc_fake_resolver_response_generator* generator,
+    grpc_channel_args* response) {
   GPR_ASSERT(generator->resolver != nullptr);
   set_response_closure_arg* closure_arg =
       (set_response_closure_arg*)gpr_zalloc(sizeof(*closure_arg));
   closure_arg->generator = generator;
-  closure_arg->next_response = grpc_channel_args_copy(next_response);
+  closure_arg->response =
+      response != nullptr ? grpc_channel_args_copy(response) : nullptr;
+  closure_arg->upon_error = true;
   GRPC_CLOSURE_SCHED(GRPC_CLOSURE_INIT(&closure_arg->set_response_closure,
-                                       set_response_closure_fn, closure_arg,
+                                       set_response_closure_locked, closure_arg,
                                        grpc_combiner_scheduler(
                                            generator->resolver->base.combiner)),
                      GRPC_ERROR_NONE);
diff --git a/src/core/ext/filters/client_channel/resolver/fake/fake_resolver.h b/src/core/ext/filters/client_channel/resolver/fake/fake_resolver.h
index a8977e5980..94f9a8e6ca 100644
--- a/src/core/ext/filters/client_channel/resolver/fake/fake_resolver.h
+++ b/src/core/ext/filters/client_channel/resolver/fake/fake_resolver.h
@@ -36,11 +36,20 @@ typedef struct grpc_fake_resolver_response_generator
 grpc_fake_resolver_response_generator*
 grpc_fake_resolver_response_generator_create();
 
-// Instruct the fake resolver associated with the \a response_generator instance
-// to trigger a new resolution for \a uri and \a args.
+// Set next response of the fake resolver associated with the \a
+// response_generator instance and trigger a new resolution.
 void grpc_fake_resolver_response_generator_set_response(
     grpc_fake_resolver_response_generator* generator,
-    grpc_channel_args* next_response);
+    grpc_channel_args* response);
+
+// Set results_upon_error of the fake resolver associated with the \a
+// response_generator instance. When fake_resolver_channel_saw_error_locked() is
+// called, results_upon_error will be returned as long as it's non-NULL,
+// otherwise the last value set by
+// grpc_fake_resolver_response_generator_set_response() will be returned.
+void grpc_fake_resolver_response_generator_set_response_upon_error(
+    grpc_fake_resolver_response_generator* generator,
+    grpc_channel_args* response);
 
 // Return a \a grpc_arg for a \a grpc_fake_resolver_response_generator instance.
 grpc_arg grpc_fake_resolver_response_generator_arg(