Merge pull request #13815 from vjpai/catcher

C++: Catch exceptions from sync method handlers rather than crashing server

BUILD

@@ -38,6 +38,16 @@ config_setting(
     values = {"define": "grpc_no_ares=true"},
 )
 
+config_setting(
+    name = "grpc_allow_exceptions",
+    values = {"define": "GRPC_ALLOW_EXCEPTIONS=1"},
+)
+
+config_setting(
+    name = "grpc_disallow_exceptions",
+    values = {"define": "GRPC_ALLOW_EXCEPTIONS=0"},
+)
+
 config_setting(
     name = "remote_execution",
     values = {"define": "GRPC_PORT_ISOLATED_RUNTIME=1"},
@@ -544,24 +554,24 @@ grpc_cc_library(
 
 grpc_cc_library(
     name = "debug_location",
-    public_hdrs = ["src/core/lib/support/debug_location.h"],
     language = "c++",
+    public_hdrs = ["src/core/lib/support/debug_location.h"],
 )
 
 grpc_cc_library(
     name = "ref_counted",
-    public_hdrs = ["src/core/lib/support/ref_counted.h"],
     language = "c++",
+    public_hdrs = ["src/core/lib/support/ref_counted.h"],
     deps = [
-        "grpc_trace",
         "debug_location",
+        "grpc_trace",
     ],
 )
 
 grpc_cc_library(
     name = "ref_counted_ptr",
-    public_hdrs = ["src/core/lib/support/ref_counted_ptr.h"],
     language = "c++",
+    public_hdrs = ["src/core/lib/support/ref_counted_ptr.h"],
 )
 
 grpc_cc_library(

CMakeLists.txt

@@ -525,6 +525,7 @@ add_dependencies(buildtests_cxx cxx_string_ref_test)
 add_dependencies(buildtests_cxx cxx_time_test)
 add_dependencies(buildtests_cxx end2end_test)
 add_dependencies(buildtests_cxx error_details_test)
+add_dependencies(buildtests_cxx exception_test)
 add_dependencies(buildtests_cxx filter_end2end_test)
 add_dependencies(buildtests_cxx generic_end2end_test)
 add_dependencies(buildtests_cxx golden_file_test)
@@ -10179,6 +10180,46 @@ target_link_libraries(error_details_test
 endif (gRPC_BUILD_TESTS)
 if (gRPC_BUILD_TESTS)
 
+add_executable(exception_test
+  test/cpp/end2end/exception_test.cc
+  third_party/googletest/googletest/src/gtest-all.cc
+  third_party/googletest/googlemock/src/gmock-all.cc
+)
+
+
+target_include_directories(exception_test
+  PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}
+  PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/include
+  PRIVATE ${_gRPC_SSL_INCLUDE_DIR}
+  PRIVATE ${PROTOBUF_ROOT_DIR}/src
+  PRIVATE ${BENCHMARK_ROOT_DIR}/include
+  PRIVATE ${ZLIB_ROOT_DIR}
+  PRIVATE ${CMAKE_CURRENT_BINARY_DIR}/third_party/zlib
+  PRIVATE ${CARES_INCLUDE_DIR}
+  PRIVATE ${CMAKE_CURRENT_BINARY_DIR}/third_party/cares/cares
+  PRIVATE ${CMAKE_CURRENT_BINARY_DIR}/third_party/gflags/include
+  PRIVATE third_party/googletest/googletest/include
+  PRIVATE third_party/googletest/googletest
+  PRIVATE third_party/googletest/googlemock/include
+  PRIVATE third_party/googletest/googlemock
+  PRIVATE ${_gRPC_PROTO_GENS_DIR}
+)
+
+target_link_libraries(exception_test
+  ${_gRPC_PROTOBUF_LIBRARIES}
+  ${_gRPC_ALLTARGETS_LIBRARIES}
+  grpc++_test_util
+  grpc_test_util
+  grpc++
+  grpc
+  gpr_test_util
+  gpr
+  ${_gRPC_GFLAGS_LIBRARIES}
+)
+
+endif (gRPC_BUILD_TESTS)
+if (gRPC_BUILD_TESTS)
+
 add_executable(filter_end2end_test
   test/cpp/end2end/filter_end2end_test.cc
   third_party/googletest/googletest/src/gtest-all.cc

Makefile

@@ -77,7 +77,6 @@ CC_opt = $(DEFAULT_CC)
 CXX_opt = $(DEFAULT_CXX)
 LD_opt = $(DEFAULT_CC)
 LDXX_opt = $(DEFAULT_CXX)
-CXXFLAGS_opt = -fno-exceptions
 CPPFLAGS_opt = -O2
 DEFINES_opt = NDEBUG
 
@@ -95,7 +94,6 @@ CC_dbg = $(DEFAULT_CC)
 CXX_dbg = $(DEFAULT_CXX)
 LD_dbg = $(DEFAULT_CC)
 LDXX_dbg = $(DEFAULT_CXX)
-CXXFLAGS_dbg = -fno-exceptions
 CPPFLAGS_dbg = -O0
 DEFINES_dbg = _DEBUG DEBUG
 
@@ -144,14 +142,14 @@ LDXX_asan-noleaks = clang++
 CPPFLAGS_asan-noleaks = -O0 -fsanitize-coverage=edge -fsanitize=address -fno-omit-frame-pointer -Wno-unused-command-line-argument -DGPR_NO_DIRECT_SYSCALLS
 LDFLAGS_asan-noleaks = -fsanitize=address
 
-VALID_CONFIG_c++-compat = 1
-CC_c++-compat = $(DEFAULT_CC)
-CXX_c++-compat = $(DEFAULT_CXX)
-LD_c++-compat = $(DEFAULT_CC)
-LDXX_c++-compat = $(DEFAULT_CXX)
-CFLAGS_c++-compat = -Wc++-compat
-CPPFLAGS_c++-compat = -O0
-DEFINES_c++-compat = _DEBUG DEBUG
+VALID_CONFIG_noexcept = 1
+CC_noexcept = $(DEFAULT_CC)
+CXX_noexcept = $(DEFAULT_CXX)
+LD_noexcept = $(DEFAULT_CC)
+LDXX_noexcept = $(DEFAULT_CXX)
+CXXFLAGS_noexcept = -fno-exceptions
+CPPFLAGS_noexcept = -O2
+DEFINES_noexcept = NDEBUG
 
 VALID_CONFIG_ubsan = 1
 REQUIRE_CUSTOM_LIBRARIES_ubsan = 1
@@ -207,6 +205,15 @@ LDXX_lto = $(DEFAULT_CXX)
 CPPFLAGS_lto = -O2
 DEFINES_lto = NDEBUG
 
+VALID_CONFIG_c++-compat = 1
+CC_c++-compat = $(DEFAULT_CC)
+CXX_c++-compat = $(DEFAULT_CXX)
+LD_c++-compat = $(DEFAULT_CC)
+LDXX_c++-compat = $(DEFAULT_CXX)
+CFLAGS_c++-compat = -Wc++-compat
+CPPFLAGS_c++-compat = -O0
+DEFINES_c++-compat = _DEBUG DEBUG
+
 VALID_CONFIG_mutrace = 1
 CC_mutrace = $(DEFAULT_CC)
 CXX_mutrace = $(DEFAULT_CXX)
@@ -1124,6 +1131,7 @@ cxx_string_ref_test: $(BINDIR)/$(CONFIG)/cxx_string_ref_test
 cxx_time_test: $(BINDIR)/$(CONFIG)/cxx_time_test
 end2end_test: $(BINDIR)/$(CONFIG)/end2end_test
 error_details_test: $(BINDIR)/$(CONFIG)/error_details_test
+exception_test: $(BINDIR)/$(CONFIG)/exception_test
 filter_end2end_test: $(BINDIR)/$(CONFIG)/filter_end2end_test
 generic_end2end_test: $(BINDIR)/$(CONFIG)/generic_end2end_test
 golden_file_test: $(BINDIR)/$(CONFIG)/golden_file_test
@@ -1572,6 +1580,7 @@ buildtests_cxx: privatelibs_cxx \
   $(BINDIR)/$(CONFIG)/cxx_time_test \
   $(BINDIR)/$(CONFIG)/end2end_test \
   $(BINDIR)/$(CONFIG)/error_details_test \
+  $(BINDIR)/$(CONFIG)/exception_test \
   $(BINDIR)/$(CONFIG)/filter_end2end_test \
   $(BINDIR)/$(CONFIG)/generic_end2end_test \
   $(BINDIR)/$(CONFIG)/golden_file_test \
@@ -1702,6 +1711,7 @@ buildtests_cxx: privatelibs_cxx \
   $(BINDIR)/$(CONFIG)/cxx_time_test \
   $(BINDIR)/$(CONFIG)/end2end_test \
   $(BINDIR)/$(CONFIG)/error_details_test \
+  $(BINDIR)/$(CONFIG)/exception_test \
   $(BINDIR)/$(CONFIG)/filter_end2end_test \
   $(BINDIR)/$(CONFIG)/generic_end2end_test \
   $(BINDIR)/$(CONFIG)/golden_file_test \
@@ -2102,6 +2112,8 @@ test_cxx: buildtests_cxx
 	$(Q) $(BINDIR)/$(CONFIG)/end2end_test || ( echo test end2end_test failed ; exit 1 )
 	$(E) "[RUN]     Testing error_details_test"
 	$(Q) $(BINDIR)/$(CONFIG)/error_details_test || ( echo test error_details_test failed ; exit 1 )
+	$(E) "[RUN]     Testing exception_test"
+	$(Q) $(BINDIR)/$(CONFIG)/exception_test || ( echo test exception_test failed ; exit 1 )
 	$(E) "[RUN]     Testing filter_end2end_test"
 	$(Q) $(BINDIR)/$(CONFIG)/filter_end2end_test || ( echo test filter_end2end_test failed ; exit 1 )
 	$(E) "[RUN]     Testing generic_end2end_test"
@@ -14974,6 +14986,49 @@ endif
 $(OBJDIR)/$(CONFIG)/test/cpp/util/error_details_test.o: $(GENDIR)/src/proto/grpc/testing/echo_messages.pb.cc $(GENDIR)/src/proto/grpc/testing/echo_messages.grpc.pb.cc
 
 
+EXCEPTION_TEST_SRC = \
+    test/cpp/end2end/exception_test.cc \
+
+EXCEPTION_TEST_OBJS = $(addprefix $(OBJDIR)/$(CONFIG)/, $(addsuffix .o, $(basename $(EXCEPTION_TEST_SRC))))
+ifeq ($(NO_SECURE),true)
+
+# You can't build secure targets if you don't have OpenSSL.
+
+$(BINDIR)/$(CONFIG)/exception_test: openssl_dep_error
+
+else
+
+
+
+
+ifeq ($(NO_PROTOBUF),true)
+
+# You can't build the protoc plugins or protobuf-enabled targets if you don't have protobuf 3.0.0+.
+
+$(BINDIR)/$(CONFIG)/exception_test: protobuf_dep_error
+
+else
+
+$(BINDIR)/$(CONFIG)/exception_test: $(PROTOBUF_DEP) $(EXCEPTION_TEST_OBJS) $(LIBDIR)/$(CONFIG)/libgrpc++_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc++.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a
+	$(E) "[LD]      Linking $@"
+	$(Q) mkdir -p `dirname $@`
+	$(Q) $(LDXX) $(LDFLAGS) $(EXCEPTION_TEST_OBJS) $(LIBDIR)/$(CONFIG)/libgrpc++_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc++.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a $(LDLIBSXX) $(LDLIBS_PROTOBUF) $(LDLIBS) $(LDLIBS_SECURE) $(GTEST_LIB) -o $(BINDIR)/$(CONFIG)/exception_test
+
+endif
+
+endif
+
+$(OBJDIR)/$(CONFIG)/test/cpp/end2end/exception_test.o:  $(LIBDIR)/$(CONFIG)/libgrpc++_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc++.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a
+
+deps_exception_test: $(EXCEPTION_TEST_OBJS:.o=.dep)
+
+ifneq ($(NO_SECURE),true)
+ifneq ($(NO_DEPS),true)
+-include $(EXCEPTION_TEST_OBJS:.o=.dep)
+endif
+endif
+
+
 FILTER_END2END_TEST_SRC = \
     test/cpp/end2end/filter_end2end_test.cc \
 

bazel/grpc_build_system.bzl

@@ -63,6 +63,10 @@ def grpc_cc_library(name, srcs = [], public_hdrs = [], hdrs = [],
     defines = select({"//:grpc_no_ares": ["GRPC_ARES=0"],
                       "//conditions:default": [],}) +
               select({"//:remote_execution":  ["GRPC_PORT_ISOLATED_RUNTIME=1"],
+                      "//conditions:default": [],}) +
+              select({"//:grpc_allow_exceptions":  ["GRPC_ALLOW_EXCEPTIONS=1"],
+                      "//:grpc_disallow_exceptions":
+                      ["GRPC_ALLOW_EXCEPTIONS=0"],
                       "//conditions:default": [],}),
     hdrs = _maybe_update_cc_library_hdrs(hdrs + public_hdrs),
     deps = deps + _get_external_deps(external_deps),

build.yaml

@@ -4007,6 +4007,19 @@ targets:
   deps:
   - grpc++_error_details
   - grpc++
+- name: exception_test
+  gtest: true
+  build: test
+  language: c++
+  src:
+  - test/cpp/end2end/exception_test.cc
+  deps:
+  - grpc++_test_util
+  - grpc_test_util
+  - grpc++
+  - grpc
+  - gpr_test_util
+  - gpr
 - name: filter_end2end_test
   gtest: true
   build: test
@@ -4927,7 +4940,6 @@ configs:
     DEFINES: NDEBUG
   dbg:
     CPPFLAGS: -O0
-    CXXFLAGS: -fno-exceptions
     DEFINES: _DEBUG DEBUG
   gcov:
     CC: gcc
@@ -4968,10 +4980,13 @@ configs:
     CPPFLAGS: -O3 -fno-omit-frame-pointer
     DEFINES: NDEBUG
     LDFLAGS: -rdynamic
-  opt:
+  noexcept:
     CPPFLAGS: -O2
     CXXFLAGS: -fno-exceptions
     DEFINES: NDEBUG
+  opt:
+    CPPFLAGS: -O2
+    DEFINES: NDEBUG
   stapprof:
     CPPFLAGS: -O2 -DGRPC_STAP_PROFILER
     DEFINES: NDEBUG

include/grpc++/impl/codegen/method_handler_impl.h

@@ -27,6 +27,27 @@
 namespace grpc {
 
 namespace internal {
+
+// Invoke the method handler, fill in the status, and
+// return whether or not we finished safely (without an exception).
+// Note that exception handling is 0-cost in most compiler/library
+// implementations (except when an exception is actually thrown),
+// so this process doesn't require additional overhead in the common case.
+// Additionally, we don't need to return if we caught an exception or not;
+// the handling is the same in either case.
+template <class Callable>
+Status CatchingFunctionHandler(Callable&& handler) {
+#if GRPC_ALLOW_EXCEPTIONS
+  try {
+    return handler();
+  } catch (...) {
+    return Status(StatusCode::UNKNOWN, "Unexpected error in RPC handling");
+  }
+#else   // GRPC_ALLOW_EXCEPTIONS
+  return handler();
+#endif  // GRPC_ALLOW_EXCEPTIONS
+}
+
 /// A wrapper class of an application provided rpc method handler.
 template <class ServiceType, class RequestType, class ResponseType>
 class RpcMethodHandler : public MethodHandler {
@@ -43,7 +64,9 @@ class RpcMethodHandler : public MethodHandler {
         param.request.bbuf_ptr(), &req);
     ResponseType rsp;
     if (status.ok()) {
-      status = func_(service_, param.server_context, &req, &rsp);
+      status = CatchingFunctionHandler([this, &param, &req, &rsp] {
+        return func_(service_, param.server_context, &req, &rsp);
+      });
     }
 
     GPR_CODEGEN_ASSERT(!param.server_context->sent_initial_metadata_);
@@ -86,7 +109,9 @@ class ClientStreamingHandler : public MethodHandler {
   void RunHandler(const HandlerParameter& param) final {
     ServerReader<RequestType> reader(param.call, param.server_context);
     ResponseType rsp;
-    Status status = func_(service_, param.server_context, &reader, &rsp);
+    Status status = CatchingFunctionHandler([this, &param, &reader, &rsp] {
+      return func_(service_, param.server_context, &reader, &rsp);
+    });
 
     GPR_CODEGEN_ASSERT(!param.server_context->sent_initial_metadata_);
     CallOpSet<CallOpSendInitialMetadata, CallOpSendMessage,
@@ -130,7 +155,9 @@ class ServerStreamingHandler : public MethodHandler {
 
     if (status.ok()) {
       ServerWriter<ResponseType> writer(param.call, param.server_context);
-      status = func_(service_, param.server_context, &req, &writer);
+      status = CatchingFunctionHandler([this, &param, &req, &writer] {
+        return func_(service_, param.server_context, &req, &writer);
+      });
     }
 
     CallOpSet<CallOpSendInitialMetadata, CallOpServerSendStatus> ops;
@@ -172,7 +199,9 @@ class TemplatedBidiStreamingHandler : public MethodHandler {
 
   void RunHandler(const HandlerParameter& param) final {
     Streamer stream(param.call, param.server_context);
-    Status status = func_(param.server_context, &stream);
+    Status status = CatchingFunctionHandler([this, &param, &stream] {
+      return func_(param.server_context, &stream);
+    });
 
     CallOpSet<CallOpSendInitialMetadata, CallOpServerSendStatus> ops;
     if (!param.server_context->sent_initial_metadata_) {

include/grpc/impl/codegen/port_platform.h

@@ -485,6 +485,21 @@ typedef unsigned __int64 uint64_t;
 #endif /* GPR_ATTRIBUTE_NO_TSAN (2) */
 #endif /* GPR_ATTRIBUTE_NO_TSAN (1) */
 
+/* GRPC_ALLOW_EXCEPTIONS should be 0 or 1 if exceptions are allowed or not */
+#ifndef GRPC_ALLOW_EXCEPTIONS
+/* If not already set, set to 1 on Windows (style guide standard) but to
+ * 0 on non-Windows platforms unless the compiler defines __EXCEPTIONS */
+#ifdef GPR_WINDOWS
+#define GRPC_ALLOW_EXCEPTIONS 1
+#else /* GPR_WINDOWS */
+#ifdef __EXCEPTIONS
+#define GRPC_ALLOW_EXCEPTIONS 1
+#else /* __EXCEPTIONS */
+#define GRPC_ALLOW_EXCEPTIONS 0
+#endif /* __EXCEPTIONS */
+#endif /* __GPR_WINDOWS */
+#endif /* GRPC_ALLOW_EXCEPTIONS */
+
 #ifndef __STDC_FORMAT_MACROS
 #define __STDC_FORMAT_MACROS
 #endif