Second approach

src/core/lib/security/credentials/plugin/plugin_credentials.cc

@@ -54,10 +54,15 @@ void grpc_plugin_credentials::pending_request_remove_locked(
   }
 }
 
+// Checks if the request has been cancelled.
+// If not, removes it from the pending list, so that it cannot be
+// cancelled out from under us.
+// When this returns, r->cancelled indicates whether the request was
+// cancelled before completion.
 void grpc_plugin_credentials::pending_request_complete(pending_request* r) {
   GPR_DEBUG_ASSERT(r->creds == this);
   gpr_mu_lock(&mu_);
-  pending_request_remove_locked(r);
+  if (!r->cancelled) pending_request_remove_locked(r);
   gpr_mu_unlock(&mu_);
   // Ref to credentials not needed anymore.
   Unref();
@@ -71,8 +76,7 @@ static grpc_error* process_plugin_result(
     char* msg;
     gpr_asprintf(&msg, "Getting metadata from plugin failed with error: %s",
                  error_details);
-    error = grpc_error_set_int(GRPC_ERROR_CREATE_FROM_COPIED_STRING(msg),
-                               GRPC_ERROR_INT_GRPC_STATUS, status);
+    error = GRPC_ERROR_CREATE_FROM_COPIED_STRING(msg);
     gpr_free(msg);
   } else {
     bool seen_illegal_header = false;
@@ -91,9 +95,7 @@ static grpc_error* process_plugin_result(
       }
     }
     if (seen_illegal_header) {
-      error = grpc_error_set_int(
-          GRPC_ERROR_CREATE_FROM_STATIC_STRING("Illegal metadata."),
-          GRPC_ERROR_INT_GRPC_STATUS, GRPC_STATUS_INTERNAL);
+      error = GRPC_ERROR_CREATE_FROM_STATIC_STRING("Illegal metadata");
     } else {
       for (size_t i = 0; i < num_md; ++i) {
         grpc_mdelem mdelem =
@@ -123,18 +125,19 @@ static void plugin_md_request_metadata_ready(void* request,
             "asynchronously",
             r->creds, r);
   }
-  // Remove request from pending list
+  // Remove request from pending list if not previously cancelled.
   r->creds->pending_request_complete(r);
   // If it has not been cancelled, process it.
-  if (r->error == GRPC_ERROR_NONE) {
-    r->error = process_plugin_result(r, md, num_md, status, error_details);
+  if (!r->cancelled) {
+    grpc_error* error =
+        process_plugin_result(r, md, num_md, status, error_details);
+    GRPC_CLOSURE_SCHED(r->on_request_metadata, error);
   } else if (GRPC_TRACE_FLAG_ENABLED(grpc_plugin_credentials_trace)) {
     gpr_log(GPR_INFO,
             "plugin_credentials[%p]: request %p: plugin was previously "
             "cancelled",
             r->creds, r);
   }
-  GRPC_CLOSURE_SCHED(r->on_request_metadata, r->error);
   gpr_free(r);
 }
 
@@ -142,11 +145,11 @@ bool grpc_plugin_credentials::get_request_metadata(
     grpc_polling_entity* pollent, grpc_auth_metadata_context context,
     grpc_credentials_mdelem_array* md_array, grpc_closure* on_request_metadata,
     grpc_error** error) {
+  bool retval = true;  // Synchronous return.
   if (plugin_.get_metadata != nullptr) {
     // Create pending_request object.
     pending_request* request =
         static_cast<pending_request*>(gpr_zalloc(sizeof(*request)));
-    request->error = GRPC_ERROR_NONE;
     request->creds = this;
     request->md_array = md_array;
     request->on_request_metadata = on_request_metadata;
@@ -180,16 +183,29 @@ bool grpc_plugin_credentials::get_request_metadata(
       return false;  // Asynchronous return.
     }
     // Returned synchronously.
-    // Remove request from pending list.
+    // Remove request from pending list if not previously cancelled.
     request->creds->pending_request_complete(request);
-    if (GRPC_TRACE_FLAG_ENABLED(grpc_plugin_credentials_trace)) {
-      gpr_log(GPR_INFO,
-              "plugin_credentials[%p]: request %p: plugin returned "
-              "synchronously",
-              this, request);
+    // If the request was cancelled, the error will have been returned
+    // asynchronously by plugin_cancel_get_request_metadata(), so return
+    // false.  Otherwise, process the result.
+    if (request->cancelled) {
+      if (GRPC_TRACE_FLAG_ENABLED(grpc_plugin_credentials_trace)) {
+        gpr_log(GPR_INFO,
+                "plugin_credentials[%p]: request %p was cancelled, error "
+                "will be returned asynchronously",
+                this, request);
+      }
+      retval = false;
+    } else {
+      if (GRPC_TRACE_FLAG_ENABLED(grpc_plugin_credentials_trace)) {
+        gpr_log(GPR_INFO,
+                "plugin_credentials[%p]: request %p: plugin returned "
+                "synchronously",
+                this, request);
+      }
+      *error = process_plugin_result(request, creds_md, num_creds_md, status,
+                                     error_details);
     }
-    *error = process_plugin_result(request, creds_md, num_creds_md, status,
-                                   error_details);
     // Clean up.
     for (size_t i = 0; i < num_creds_md; ++i) {
       grpc_slice_unref_internal(creds_md[i].key);
@@ -198,7 +214,7 @@ bool grpc_plugin_credentials::get_request_metadata(
     gpr_free((void*)error_details);
     gpr_free(request);
   }
-  return true;  // Synchronous return.
+  return retval;
 }
 
 void grpc_plugin_credentials::cancel_get_request_metadata(
@@ -211,11 +227,15 @@ void grpc_plugin_credentials::cancel_get_request_metadata(
         gpr_log(GPR_INFO, "plugin_credentials[%p]: cancelling request %p", this,
                 pending_request);
       }
-      pending_request->error = error;
+      pending_request->cancelled = true;
+      GRPC_CLOSURE_SCHED(pending_request->on_request_metadata,
+                         GRPC_ERROR_REF(error));
+      pending_request_remove_locked(pending_request);
       break;
     }
   }
   gpr_mu_unlock(&mu_);
+  GRPC_ERROR_UNREF(error);
 }
 
 grpc_plugin_credentials::grpc_plugin_credentials(

src/core/lib/security/credentials/plugin/plugin_credentials.h

@@ -31,7 +31,7 @@ extern grpc_core::TraceFlag grpc_plugin_credentials_trace;
 struct grpc_plugin_credentials final : public grpc_call_credentials {
  public:
   struct pending_request {
-    grpc_error* error;
+    bool cancelled;
     struct grpc_plugin_credentials* creds;
     grpc_credentials_mdelem_array* md_array;
     grpc_closure* on_request_metadata;
@@ -51,6 +51,11 @@ struct grpc_plugin_credentials final : public grpc_call_credentials {
   void cancel_get_request_metadata(grpc_credentials_mdelem_array* md_array,
                                    grpc_error* error) override;
 
+  // Checks if the request has been cancelled.
+  // If not, removes it from the pending list, so that it cannot be
+  // cancelled out from under us.
+  // When this returns, r->cancelled indicates whether the request was
+  // cancelled before completion.
   void pending_request_complete(pending_request* r);
 
  private:

src/core/lib/security/transport/auth_filters.h

@@ -32,6 +32,9 @@ void grpc_auth_metadata_context_build(
     const grpc_slice& call_method, grpc_auth_context* auth_context,
     grpc_auth_metadata_context* auth_md_context);
 
+void grpc_auth_metadata_context_copy(grpc_auth_metadata_context* from,
+                                     grpc_auth_metadata_context* to);
+
 void grpc_auth_metadata_context_reset(grpc_auth_metadata_context* context);
 
 #endif /* GRPC_CORE_LIB_SECURITY_TRANSPORT_AUTH_FILTERS_H */

src/core/lib/security/transport/client_auth_filter.cc

@@ -112,6 +112,20 @@ struct call_data {
 
 }  // namespace
 
+void grpc_auth_metadata_context_copy(grpc_auth_metadata_context* from,
+                                     grpc_auth_metadata_context* to) {
+  grpc_auth_metadata_context_reset(to);
+  to->channel_auth_context = from->channel_auth_context;
+  if (to->channel_auth_context != nullptr) {
+    const_cast<grpc_auth_context*>(to->channel_auth_context)
+        ->Ref(DEBUG_LOCATION, "grpc_auth_metadata_context_copy")
+        .release();
+  }
+  to->service_url =
+      (from->service_url == nullptr) ? nullptr : strdup(from->service_url);
+  to->method_name =
+      (from->method_name == nullptr) ? nullptr : strdup(from->method_name);
+}
 void grpc_auth_metadata_context_reset(
     grpc_auth_metadata_context* auth_md_context) {
   if (auth_md_context->service_url != nullptr) {
@@ -160,6 +174,8 @@ static void on_credentials_metadata(void* arg, grpc_error* input_error) {
   if (error == GRPC_ERROR_NONE) {
     grpc_call_next_op(elem, batch);
   } else {
+    error = grpc_error_set_int(error, GRPC_ERROR_INT_GRPC_STATUS,
+                               GRPC_STATUS_UNAVAILABLE);
     grpc_transport_stream_op_batch_finish_with_failure(batch, error,
                                                        calld->call_combiner);
   }

src/cpp/client/secure_credentials.cc

@@ -22,6 +22,7 @@
 #include <grpcpp/channel.h>
 #include <grpcpp/impl/grpc_library.h>
 #include <grpcpp/support/channel_arguments.h>
+#include "src/core/lib/security/transport/auth_filters.h"
 #include "src/cpp/client/create_channel_internal.h"
 #include "src/cpp/common/secure_auth_context.h"
 
@@ -247,10 +248,13 @@ int MetadataCredentialsPluginWrapper::GetMetadata(
     return true;
   }
   if (w->plugin_->IsBlocking()) {
+    grpc_auth_metadata_context context_copy = grpc_auth_metadata_context();
+    grpc_auth_metadata_context_copy(&context, &context_copy);
     // Asynchronous return.
-    w->thread_pool_->Add([w, context, cb, user_data] {
+    w->thread_pool_->Add([w, context_copy, cb, user_data]() mutable {
       w->MetadataCredentialsPluginWrapper::InvokePlugin(
-          context, cb, user_data, nullptr, nullptr, nullptr, nullptr);
+          context_copy, cb, user_data, nullptr, nullptr, nullptr, nullptr);
+      grpc_auth_metadata_context_reset(&context_copy);
     });
     return 0;
   } else {

test/cpp/end2end/end2end_test.cc

@@ -1849,7 +1849,7 @@ TEST_P(SecureEnd2endTest, AuthMetadataPluginKeyFailure) {
 
   Status s = stub_->Echo(&context, request, &response);
   EXPECT_FALSE(s.ok());
-  EXPECT_EQ(s.error_code(), StatusCode::INTERNAL);
+  EXPECT_EQ(s.error_code(), StatusCode::UNAVAILABLE);
 }
 
 TEST_P(SecureEnd2endTest, AuthMetadataPluginValueFailure) {
@@ -1867,7 +1867,7 @@ TEST_P(SecureEnd2endTest, AuthMetadataPluginValueFailure) {
 
   Status s = stub_->Echo(&context, request, &response);
   EXPECT_FALSE(s.ok());
-  EXPECT_EQ(s.error_code(), StatusCode::INTERNAL);
+  EXPECT_EQ(s.error_code(), StatusCode::UNAVAILABLE);
 }
 
 TEST_P(SecureEnd2endTest, AuthMetadataPluginWithDeadline) {
@@ -1888,7 +1888,7 @@ TEST_P(SecureEnd2endTest, AuthMetadataPluginWithDeadline) {
 
   Status s = stub_->Echo(&context, request, &response);
   if (!s.ok()) {
-    EXPECT_EQ(StatusCode::DEADLINE_EXCEEDED, s.error_code());
+    EXPECT_EQ(StatusCode::UNAVAILABLE, s.error_code());
   }
 }
 
@@ -1912,7 +1912,7 @@ TEST_P(SecureEnd2endTest, AuthMetadataPluginWithCancel) {
   });
   Status s = stub_->Echo(&context, request, &response);
   if (!s.ok()) {
-    EXPECT_EQ(StatusCode::CANCELLED, s.error_code());
+    EXPECT_EQ(StatusCode::UNAVAILABLE, s.error_code());
   }
   cancel_thread.join();
 }
@@ -1933,7 +1933,7 @@ TEST_P(SecureEnd2endTest, NonBlockingAuthMetadataPluginFailure) {
 
   Status s = stub_->Echo(&context, request, &response);
   EXPECT_FALSE(s.ok());
-  EXPECT_EQ(s.error_code(), StatusCode::NOT_FOUND);
+  EXPECT_EQ(s.error_code(), StatusCode::UNAVAILABLE);
   EXPECT_EQ(s.error_message(),
             grpc::string("Getting metadata from plugin failed with error: ") +
                 kTestCredsPluginErrorMsg);
@@ -1997,7 +1997,7 @@ TEST_P(SecureEnd2endTest, BlockingAuthMetadataPluginFailure) {
 
   Status s = stub_->Echo(&context, request, &response);
   EXPECT_FALSE(s.ok());
-  EXPECT_EQ(s.error_code(), StatusCode::NOT_FOUND);
+  EXPECT_EQ(s.error_code(), StatusCode::UNAVAILABLE);
   EXPECT_EQ(s.error_message(),
             grpc::string("Getting metadata from plugin failed with error: ") +
                 kTestCredsPluginErrorMsg);