Merge pull request grpc#24577 from yang-g/init_failure

Do not crash if server filter fails at ChannelData::Init

CMakeLists.txt

@@ -1042,9 +1042,9 @@ add_library(end2end_nosec_tests
   test/core/end2end/tests/default_host.cc
   test/core/end2end/tests/disappearing_server.cc
   test/core/end2end/tests/empty_batch.cc
-  test/core/end2end/tests/filter_call_init_fails.cc
   test/core/end2end/tests/filter_causes_close.cc
   test/core/end2end/tests/filter_context.cc
+  test/core/end2end/tests/filter_init_fails.cc
   test/core/end2end/tests/filter_latency.cc
   test/core/end2end/tests/filter_status_code.cc
   test/core/end2end/tests/graceful_server_shutdown.cc
@@ -1176,9 +1176,9 @@ add_library(end2end_tests
   test/core/end2end/tests/default_host.cc
   test/core/end2end/tests/disappearing_server.cc
   test/core/end2end/tests/empty_batch.cc
-  test/core/end2end/tests/filter_call_init_fails.cc
   test/core/end2end/tests/filter_causes_close.cc
   test/core/end2end/tests/filter_context.cc
+  test/core/end2end/tests/filter_init_fails.cc
   test/core/end2end/tests/filter_latency.cc
   test/core/end2end/tests/filter_status_code.cc
   test/core/end2end/tests/graceful_server_shutdown.cc

build_autogenerated.yaml

@@ -55,9 +55,9 @@ libs:
   - test/core/end2end/tests/default_host.cc
   - test/core/end2end/tests/disappearing_server.cc
   - test/core/end2end/tests/empty_batch.cc
-  - test/core/end2end/tests/filter_call_init_fails.cc
   - test/core/end2end/tests/filter_causes_close.cc
   - test/core/end2end/tests/filter_context.cc
+  - test/core/end2end/tests/filter_init_fails.cc
   - test/core/end2end/tests/filter_latency.cc
   - test/core/end2end/tests/filter_status_code.cc
   - test/core/end2end/tests/graceful_server_shutdown.cc
@@ -165,9 +165,9 @@ libs:
   - test/core/end2end/tests/default_host.cc
   - test/core/end2end/tests/disappearing_server.cc
   - test/core/end2end/tests/empty_batch.cc
-  - test/core/end2end/tests/filter_call_init_fails.cc
   - test/core/end2end/tests/filter_causes_close.cc
   - test/core/end2end/tests/filter_context.cc
+  - test/core/end2end/tests/filter_init_fails.cc
   - test/core/end2end/tests/filter_latency.cc
   - test/core/end2end/tests/filter_status_code.cc
   - test/core/end2end/tests/graceful_server_shutdown.cc

gRPC-Core.podspec

@@ -1961,9 +1961,9 @@ Pod::Spec.new do |s|
                       'test/core/end2end/tests/default_host.cc',
                       'test/core/end2end/tests/disappearing_server.cc',
                       'test/core/end2end/tests/empty_batch.cc',
-                      'test/core/end2end/tests/filter_call_init_fails.cc',
                       'test/core/end2end/tests/filter_causes_close.cc',
                       'test/core/end2end/tests/filter_context.cc',
+                      'test/core/end2end/tests/filter_init_fails.cc',
                       'test/core/end2end/tests/filter_latency.cc',
                       'test/core/end2end/tests/filter_status_code.cc',
                       'test/core/end2end/tests/graceful_server_shutdown.cc',

grpc.gyp

@@ -210,9 +210,9 @@
         'test/core/end2end/tests/default_host.cc',
         'test/core/end2end/tests/disappearing_server.cc',
         'test/core/end2end/tests/empty_batch.cc',
-        'test/core/end2end/tests/filter_call_init_fails.cc',
         'test/core/end2end/tests/filter_causes_close.cc',
         'test/core/end2end/tests/filter_context.cc',
+        'test/core/end2end/tests/filter_init_fails.cc',
         'test/core/end2end/tests/filter_latency.cc',
         'test/core/end2end/tests/filter_status_code.cc',
         'test/core/end2end/tests/graceful_server_shutdown.cc',
@@ -313,9 +313,9 @@
         'test/core/end2end/tests/default_host.cc',
         'test/core/end2end/tests/disappearing_server.cc',
         'test/core/end2end/tests/empty_batch.cc',
-        'test/core/end2end/tests/filter_call_init_fails.cc',
         'test/core/end2end/tests/filter_causes_close.cc',
         'test/core/end2end/tests/filter_context.cc',
+        'test/core/end2end/tests/filter_init_fails.cc',
         'test/core/end2end/tests/filter_latency.cc',
         'test/core/end2end/tests/filter_status_code.cc',
         'test/core/end2end/tests/graceful_server_shutdown.cc',

src/core/ext/transport/chttp2/client/insecure/channel_create.cc

@@ -49,9 +49,13 @@ class Chttp2InsecureClientChannelFactory : public ClientChannelFactory {
 
 namespace {
 
-grpc_channel* CreateChannel(const char* target, const grpc_channel_args* args) {
+grpc_channel* CreateChannel(const char* target, const grpc_channel_args* args,
+                            grpc_error** error) {
   if (target == nullptr) {
     gpr_log(GPR_ERROR, "cannot create channel with NULL target name");
+    if (error != nullptr) {
+      *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING("channel target is NULL");
+    }
     return nullptr;
   }
   // Add channel arg containing the server URI.
@@ -62,8 +66,8 @@ grpc_channel* CreateChannel(const char* target, const grpc_channel_args* args) {
   const char* to_remove[] = {GRPC_ARG_SERVER_URI};
   grpc_channel_args* new_args =
       grpc_channel_args_copy_and_add_and_remove(args, to_remove, 1, &arg, 1);
-  grpc_channel* channel =
-      grpc_channel_create(target, new_args, GRPC_CLIENT_CHANNEL, nullptr);
+  grpc_channel* channel = grpc_channel_create(
+      target, new_args, GRPC_CLIENT_CHANNEL, nullptr, nullptr, error);
   grpc_channel_args_destroy(new_args);
   return channel;
 }
@@ -101,12 +105,20 @@ grpc_channel* grpc_insecure_channel_create(const char* target,
   const char* arg_to_remove = arg.key;
   grpc_channel_args* new_args = grpc_channel_args_copy_and_add_and_remove(
       args, &arg_to_remove, 1, &arg, 1);
+  grpc_error* error = GRPC_ERROR_NONE;
   // Create channel.
-  grpc_channel* channel = grpc_core::CreateChannel(target, new_args);
+  grpc_channel* channel = grpc_core::CreateChannel(target, new_args, &error);
   // Clean up.
   grpc_channel_args_destroy(new_args);
-  return channel != nullptr ? channel
-                            : grpc_lame_client_channel_create(
-                                  target, GRPC_STATUS_INTERNAL,
-                                  "Failed to create client channel");
+  if (channel == nullptr) {
+    intptr_t integer;
+    grpc_status_code status = GRPC_STATUS_INTERNAL;
+    if (grpc_error_get_int(error, GRPC_ERROR_INT_GRPC_STATUS, &integer)) {
+      status = static_cast<grpc_status_code>(integer);
+    }
+    GRPC_ERROR_UNREF(error);
+    channel = grpc_lame_client_channel_create(
+        target, status, "Failed to create client channel");
+  }
+  return channel;
 }

src/core/ext/transport/chttp2/client/insecure/channel_create_posix.cc

@@ -55,17 +55,27 @@ grpc_channel* grpc_insecure_channel_create_from_fd(
   grpc_transport* transport =
       grpc_create_chttp2_transport(final_args, client, true);
   GPR_ASSERT(transport);
-  grpc_channel* channel = grpc_channel_create(
-      target, final_args, GRPC_CLIENT_DIRECT_CHANNEL, transport);
+  grpc_error* error = nullptr;
+  grpc_channel* channel =
+      grpc_channel_create(target, final_args, GRPC_CLIENT_DIRECT_CHANNEL,
+                          transport, nullptr, &error);
   grpc_channel_args_destroy(final_args);
-  grpc_chttp2_transport_start_reading(transport, nullptr, nullptr);
+  if (channel != nullptr) {
+    grpc_chttp2_transport_start_reading(transport, nullptr, nullptr);
+    grpc_core::ExecCtx::Get()->Flush();
+  } else {
+    intptr_t integer;
+    grpc_status_code status = GRPC_STATUS_INTERNAL;
+    if (grpc_error_get_int(error, GRPC_ERROR_INT_GRPC_STATUS, &integer)) {
+      status = static_cast<grpc_status_code>(integer);
+    }
+    GRPC_ERROR_UNREF(error);
+    grpc_transport_destroy(transport);
+    channel = grpc_lame_client_channel_create(
+        target, status, "Failed to create client channel");
+  }
 
-  grpc_core::ExecCtx::Get()->Flush();
-
-  return channel != nullptr ? channel
-                            : grpc_lame_client_channel_create(
-                                  target, GRPC_STATUS_INTERNAL,
-                                  "Failed to create client channel");
+  return channel;
 }
 
 #else  // !GPR_SUPPORT_CHANNELS_FROM_FD

src/core/ext/transport/chttp2/client/secure/secure_channel_create.cc

@@ -127,9 +127,13 @@ class Chttp2SecureClientChannelFactory : public ClientChannelFactory {
 
 namespace {
 
-grpc_channel* CreateChannel(const char* target, const grpc_channel_args* args) {
+grpc_channel* CreateChannel(const char* target, const grpc_channel_args* args,
+                            grpc_error** error) {
   if (target == nullptr) {
     gpr_log(GPR_ERROR, "cannot create channel with NULL target name");
+    if (error != nullptr) {
+      *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING("channel target is NULL");
+    }
     return nullptr;
   }
   // Add channel arg containing the server URI.
@@ -140,8 +144,8 @@ grpc_channel* CreateChannel(const char* target, const grpc_channel_args* args) {
   const char* to_remove[] = {GRPC_ARG_SERVER_URI};
   grpc_channel_args* new_args =
       grpc_channel_args_copy_and_add_and_remove(args, to_remove, 1, &arg, 1);
-  grpc_channel* channel =
-      grpc_channel_create(target, new_args, GRPC_CLIENT_CHANNEL, nullptr);
+  grpc_channel* channel = grpc_channel_create(
+      target, new_args, GRPC_CLIENT_CHANNEL, nullptr, nullptr, error);
   grpc_channel_args_destroy(new_args);
   return channel;
 }
@@ -176,6 +180,7 @@ grpc_channel* grpc_secure_channel_create(grpc_channel_credentials* creds,
       4, ((void*)creds, target, (void*)args, (void*)reserved));
   GPR_ASSERT(reserved == nullptr);
   grpc_channel* channel = nullptr;
+  grpc_error* error = GRPC_ERROR_NONE;
   if (creds != nullptr) {
     // Add channel args containing the client channel factory and channel
     // credentials.
@@ -189,12 +194,19 @@ grpc_channel* grpc_secure_channel_create(grpc_channel_credentials* creds,
         args, &arg_to_remove, 1, args_to_add, GPR_ARRAY_SIZE(args_to_add));
     new_args = creds->update_arguments(new_args);
     // Create channel.
-    channel = grpc_core::CreateChannel(target, new_args);
+    channel = grpc_core::CreateChannel(target, new_args, &error);
     // Clean up.
     grpc_channel_args_destroy(new_args);
   }
-  return channel != nullptr ? channel
-                            : grpc_lame_client_channel_create(
-                                  target, GRPC_STATUS_INTERNAL,
-                                  "Failed to create secure client channel");
+  if (channel == nullptr) {
+    intptr_t integer;
+    grpc_status_code status = GRPC_STATUS_INTERNAL;
+    if (grpc_error_get_int(error, GRPC_ERROR_INT_GRPC_STATUS, &integer)) {
+      status = static_cast<grpc_status_code>(integer);
+    }
+    GRPC_ERROR_UNREF(error);
+    channel = grpc_lame_client_channel_create(
+        target, status, "Failed to create secure client channel");
+  }
+  return channel;
 }

src/core/ext/transport/chttp2/server/chttp2_server.cc

@@ -227,31 +227,47 @@ void Chttp2ServerListener::ConnectionState::OnHandshakeDone(void* arg,
       if (args->endpoint != nullptr) {
         grpc_transport* transport = grpc_create_chttp2_transport(
             args->args, args->endpoint, false, resource_user);
-        self->listener_->server_->SetupTransport(
+        grpc_error* channel_init_err = self->listener_->server_->SetupTransport(
             transport, self->accepting_pollset_, args->args,
             grpc_chttp2_transport_get_socket_node(transport), resource_user);
-        // Use notify_on_receive_settings callback to enforce the
-        // handshake deadline.
-        // Note: The reinterpret_cast<>s here are safe, because
-        // grpc_chttp2_transport is a C-style extension of
-        // grpc_transport, so this is morally equivalent of a
-        // static_cast<> to a derived class.
-        // TODO(roth): Change to static_cast<> when we C++-ify the
-        // transport API.
-        self->transport_ = reinterpret_cast<grpc_chttp2_transport*>(transport);
-        self->Ref().release();  // Held by OnReceiveSettings().
-        GRPC_CLOSURE_INIT(&self->on_receive_settings_, OnReceiveSettings, self,
-                          grpc_schedule_on_exec_ctx);
-        grpc_chttp2_transport_start_reading(transport, args->read_buffer,
-                                            &self->on_receive_settings_);
-        grpc_channel_args_destroy(args->args);
-        self->Ref().release();  // Held by OnTimeout().
-        GRPC_CHTTP2_REF_TRANSPORT(
-            reinterpret_cast<grpc_chttp2_transport*>(transport),
-            "receive settings timeout");
-        GRPC_CLOSURE_INIT(&self->on_timeout_, OnTimeout, self,
-                          grpc_schedule_on_exec_ctx);
-        grpc_timer_init(&self->timer_, self->deadline_, &self->on_timeout_);
+        if (channel_init_err == GRPC_ERROR_NONE) {
+          // Use notify_on_receive_settings callback to enforce the
+          // handshake deadline.
+          // Note: The reinterpret_cast<>s here are safe, because
+          // grpc_chttp2_transport is a C-style extension of
+          // grpc_transport, so this is morally equivalent of a
+          // static_cast<> to a derived class.
+          // TODO(roth): Change to static_cast<> when we C++-ify the
+          // transport API.
+          self->transport_ =
+              reinterpret_cast<grpc_chttp2_transport*>(transport);
+          self->Ref().release();  // Held by OnReceiveSettings().
+          GRPC_CLOSURE_INIT(&self->on_receive_settings_, OnReceiveSettings,
+                            self, grpc_schedule_on_exec_ctx);
+          grpc_chttp2_transport_start_reading(transport, args->read_buffer,
+                                              &self->on_receive_settings_);
+          grpc_channel_args_destroy(args->args);
+          self->Ref().release();  // Held by OnTimeout().
+          GRPC_CHTTP2_REF_TRANSPORT(
+              reinterpret_cast<grpc_chttp2_transport*>(transport),
+              "receive settings timeout");
+          GRPC_CLOSURE_INIT(&self->on_timeout_, OnTimeout, self,
+                            grpc_schedule_on_exec_ctx);
+          grpc_timer_init(&self->timer_, self->deadline_, &self->on_timeout_);
+        } else {
+          // Failed to create channel from transport. Clean up.
+          gpr_log(GPR_ERROR, "Failed to create channel: %s",
+                  grpc_error_string(channel_init_err));
+          GRPC_ERROR_UNREF(channel_init_err);
+          grpc_transport_destroy(transport);
+          grpc_slice_buffer_destroy_internal(args->read_buffer);
+          gpr_free(args->read_buffer);
+          if (resource_user != nullptr) {
+            grpc_resource_user_free(resource_user,
+                                    GRPC_RESOURCE_QUOTA_CHANNEL_SIZE);
+          }
+          grpc_channel_args_destroy(args->args);
+        }
       } else {
         if (resource_user != nullptr) {
           grpc_resource_user_free(resource_user,

src/core/ext/transport/chttp2/server/insecure/server_chttp2_posix.cc

@@ -51,12 +51,19 @@ void grpc_server_add_insecure_channel_from_fd(grpc_server* server,
   grpc_transport* transport = grpc_create_chttp2_transport(
       server_args, server_endpoint, false /* is_client */);
 
-  for (grpc_pollset* pollset : core_server->pollsets()) {
-    grpc_endpoint_add_to_pollset(server_endpoint, pollset);
+  grpc_error* error =
+      core_server->SetupTransport(transport, nullptr, server_args, nullptr);
+  if (error == GRPC_ERROR_NONE) {
+    for (grpc_pollset* pollset : core_server->pollsets()) {
+      grpc_endpoint_add_to_pollset(server_endpoint, pollset);
+    }
+    grpc_chttp2_transport_start_reading(transport, nullptr, nullptr);
+  } else {
+    gpr_log(GPR_ERROR, "Failed to create channel: %s",
+            grpc_error_string(error));
+    GRPC_ERROR_UNREF(error);
+    grpc_transport_destroy(transport);
   }
-
-  core_server->SetupTransport(transport, nullptr, server_args, nullptr);
-  grpc_chttp2_transport_start_reading(transport, nullptr, nullptr);
 }
 
 #else  // !GPR_SUPPORT_CHANNELS_FROM_FD

src/core/ext/transport/inproc/inproc_transport.cc

@@ -1292,10 +1292,43 @@ grpc_channel* grpc_inproc_channel_create(grpc_server* server,
                            client_args);
 
   // TODO(ncteisen): design and support channelz GetSocket for inproc.
-  server->core_server->SetupTransport(server_transport, nullptr, server_args,
-                                      nullptr);
-  grpc_channel* channel = grpc_channel_create(
-      "inproc", client_args, GRPC_CLIENT_DIRECT_CHANNEL, client_transport);
+  grpc_error* error = server->core_server->SetupTransport(
+      server_transport, nullptr, server_args, nullptr);
+  grpc_channel* channel = nullptr;
+  if (error == GRPC_ERROR_NONE) {
+    channel =
+        grpc_channel_create("inproc", client_args, GRPC_CLIENT_DIRECT_CHANNEL,
+                            client_transport, nullptr, &error);
+    if (error != GRPC_ERROR_NONE) {
+      GPR_ASSERT(!channel);
+      gpr_log(GPR_ERROR, "Failed to create client channel: %s",
+              grpc_error_string(error));
+      intptr_t integer;
+      grpc_status_code status = GRPC_STATUS_INTERNAL;
+      if (grpc_error_get_int(error, GRPC_ERROR_INT_GRPC_STATUS, &integer)) {
+        status = static_cast<grpc_status_code>(integer);
+      }
+      GRPC_ERROR_UNREF(error);
+      // client_transport was destroyed when grpc_channel_create saw an error.
+      grpc_transport_destroy(server_transport);
+      channel = grpc_lame_client_channel_create(
+          nullptr, status, "Failed to create client channel");
+    }
+  } else {
+    GPR_ASSERT(!channel);
+    gpr_log(GPR_ERROR, "Failed to create server channel: %s",
+            grpc_error_string(error));
+    intptr_t integer;
+    grpc_status_code status = GRPC_STATUS_INTERNAL;
+    if (grpc_error_get_int(error, GRPC_ERROR_INT_GRPC_STATUS, &integer)) {
+      status = static_cast<grpc_status_code>(integer);
+    }
+    GRPC_ERROR_UNREF(error);
+    grpc_transport_destroy(client_transport);
+    grpc_transport_destroy(server_transport);
+    channel = grpc_lame_client_channel_create(
+        nullptr, status, "Failed to create server channel");
+  }
 
   // Free up created channel args
   grpc_channel_args_destroy(server_args);