Merge pull request #17036 from vjpai/interceptor_arena

Allow interceptors to use arena allocation
6 years ago · ccfd919190
parent 9887feed46 ac10426163
commit ccfd919190
8 changed files with 170 additions and 11 deletions
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@ -665,6 +665,7 @@ add_dependencies(buildtests_cxx server_crash_test)
 endif()
 add_dependencies(buildtests_cxx server_crash_test_client)
 add_dependencies(buildtests_cxx server_early_return_test)
+add_dependencies(buildtests_cxx server_interceptors_end2end_test)
 add_dependencies(buildtests_cxx server_request_call_test)
 add_dependencies(buildtests_cxx shutdown_test)
 add_dependencies(buildtests_cxx slice_hash_table_test)
@ -15339,6 +15340,47 @@ target_link_libraries(server_early_return_test
 )


+endif (gRPC_BUILD_TESTS)
+if (gRPC_BUILD_TESTS)
+
+add_executable(server_interceptors_end2end_test
+  test/cpp/end2end/server_interceptors_end2end_test.cc
+  third_party/googletest/googletest/src/gtest-all.cc
+  third_party/googletest/googlemock/src/gmock-all.cc
+)
+
+
+target_include_directories(server_interceptors_end2end_test
+  PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}
+  PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/include
+  PRIVATE ${_gRPC_SSL_INCLUDE_DIR}
+  PRIVATE ${_gRPC_PROTOBUF_INCLUDE_DIR}
+  PRIVATE ${_gRPC_ZLIB_INCLUDE_DIR}
+  PRIVATE ${_gRPC_BENCHMARK_INCLUDE_DIR}
+  PRIVATE ${_gRPC_CARES_INCLUDE_DIR}
+  PRIVATE ${_gRPC_GFLAGS_INCLUDE_DIR}
+  PRIVATE ${_gRPC_ADDRESS_SORTING_INCLUDE_DIR}
+  PRIVATE ${_gRPC_NANOPB_INCLUDE_DIR}
+  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(server_interceptors_end2end_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)

--- a/48
+++ b/48
@ -1230,6 +1230,7 @@ server_context_test_spouse_test: $(BINDIR)/$(CONFIG)/server_context_test_spouse_
 server_crash_test: $(BINDIR)/$(CONFIG)/server_crash_test
 server_crash_test_client: $(BINDIR)/$(CONFIG)/server_crash_test_client
 server_early_return_test: $(BINDIR)/$(CONFIG)/server_early_return_test
+server_interceptors_end2end_test: $(BINDIR)/$(CONFIG)/server_interceptors_end2end_test
 server_request_call_test: $(BINDIR)/$(CONFIG)/server_request_call_test
 shutdown_test: $(BINDIR)/$(CONFIG)/shutdown_test
 slice_hash_table_test: $(BINDIR)/$(CONFIG)/slice_hash_table_test
@ -1727,6 +1728,7 @@ buildtests_cxx: privatelibs_cxx \
  $(BINDIR)/$(CONFIG)/server_crash_test \
  $(BINDIR)/$(CONFIG)/server_crash_test_client \
  $(BINDIR)/$(CONFIG)/server_early_return_test \
+  $(BINDIR)/$(CONFIG)/server_interceptors_end2end_test \
  $(BINDIR)/$(CONFIG)/server_request_call_test \
  $(BINDIR)/$(CONFIG)/shutdown_test \
  $(BINDIR)/$(CONFIG)/slice_hash_table_test \
@ -1909,6 +1911,7 @@ buildtests_cxx: privatelibs_cxx \
  $(BINDIR)/$(CONFIG)/server_crash_test \
  $(BINDIR)/$(CONFIG)/server_crash_test_client \
  $(BINDIR)/$(CONFIG)/server_early_return_test \
+  $(BINDIR)/$(CONFIG)/server_interceptors_end2end_test \
  $(BINDIR)/$(CONFIG)/server_request_call_test \
  $(BINDIR)/$(CONFIG)/shutdown_test \
  $(BINDIR)/$(CONFIG)/slice_hash_table_test \
@ -2401,6 +2404,8 @@ test_cxx: buildtests_cxx
 	$(Q) $(BINDIR)/$(CONFIG)/server_crash_test || ( echo test server_crash_test failed ; exit 1 )
 	$(E) "[RUN]     Testing server_early_return_test"
 	$(Q) $(BINDIR)/$(CONFIG)/server_early_return_test || ( echo test server_early_return_test failed ; exit 1 )
+	$(E) "[RUN]     Testing server_interceptors_end2end_test"
+	$(Q) $(BINDIR)/$(CONFIG)/server_interceptors_end2end_test || ( echo test server_interceptors_end2end_test failed ; exit 1 )
 	$(E) "[RUN]     Testing server_request_call_test"
 	$(Q) $(BINDIR)/$(CONFIG)/server_request_call_test || ( echo test server_request_call_test failed ; exit 1 )
 	$(E) "[RUN]     Testing shutdown_test"
@ -20137,6 +20142,49 @@ endif
 endif


+SERVER_INTERCEPTORS_END2END_TEST_SRC = \
+    test/cpp/end2end/server_interceptors_end2end_test.cc \
+
+SERVER_INTERCEPTORS_END2END_TEST_OBJS = $(addprefix $(OBJDIR)/$(CONFIG)/, $(addsuffix .o, $(basename $(SERVER_INTERCEPTORS_END2END_TEST_SRC))))
+ifeq ($(NO_SECURE),true)
+
+# You can't build secure targets if you don't have OpenSSL.
+
+$(BINDIR)/$(CONFIG)/server_interceptors_end2end_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.5.0+.
+
+$(BINDIR)/$(CONFIG)/server_interceptors_end2end_test: protobuf_dep_error
+
+else
+
+$(BINDIR)/$(CONFIG)/server_interceptors_end2end_test: $(PROTOBUF_DEP) $(SERVER_INTERCEPTORS_END2END_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) $(SERVER_INTERCEPTORS_END2END_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)/server_interceptors_end2end_test
+
+endif
+
+endif
+
+$(OBJDIR)/$(CONFIG)/test/cpp/end2end/server_interceptors_end2end_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_server_interceptors_end2end_test: $(SERVER_INTERCEPTORS_END2END_TEST_OBJS:.o=.dep)
+
+ifneq ($(NO_SECURE),true)
+ifneq ($(NO_DEPS),true)
+-include $(SERVER_INTERCEPTORS_END2END_TEST_OBJS:.o=.dep)
+endif
+endif
+
+
 SERVER_REQUEST_CALL_TEST_SRC = \
    $(GENDIR)/src/proto/grpc/testing/echo_messages.pb.cc $(GENDIR)/src/proto/grpc/testing/echo_messages.grpc.pb.cc \
    $(GENDIR)/src/proto/grpc/testing/echo.pb.cc $(GENDIR)/src/proto/grpc/testing/echo.grpc.pb.cc \
--- a/build.yaml
+++ b/build.yaml
@ -5456,6 +5456,22 @@ targets:
  - grpc
  - gpr_test_util
  - gpr
+- name: server_interceptors_end2end_test
+  gtest: true
+  cpu_cost: 0.5
+  build: test
+  language: c++
+  headers:
+  - test/cpp/end2end/interceptors_util.h
+  src:
+  - test/cpp/end2end/server_interceptors_end2end_test.cc
+  deps:
+  - grpc++_test_util
+  - grpc_test_util
+  - grpc++
+  - grpc
+  - gpr_test_util
+  - gpr
 - name: server_request_call_test
  gtest: true
  build: test
--- a/include/grpcpp/impl/codegen/method_handler_impl.h
+++ b/include/grpcpp/impl/codegen/method_handler_impl.h
@ -66,7 +66,7 @@ class RpcMethodHandler : public MethodHandler {
        return func_(service_, param.server_context,
                     static_cast<RequestType*>(param.request), &rsp);
      });
-      delete static_cast<RequestType*>(param.request);
+      static_cast<RequestType*>(param.request)->~RequestType();
    }

    GPR_CODEGEN_ASSERT(!param.server_context->sent_initial_metadata_);
@ -86,16 +86,18 @@ class RpcMethodHandler : public MethodHandler {
    param.call->cq()->Pluck(&ops);
  }

-  void* Deserialize(grpc_byte_buffer* req, Status* status) final {
+  void* Deserialize(grpc_call* call, grpc_byte_buffer* req,
+                    Status* status) final {
    ByteBuffer buf;
    buf.set_buffer(req);
-    auto* request = new RequestType();
+    auto* request = new (g_core_codegen_interface->grpc_call_arena_alloc(
+        call, sizeof(RequestType))) RequestType();
    *status = SerializationTraits<RequestType>::Deserialize(&buf, request);
    buf.Release();
    if (status->ok()) {
      return request;
    }
-    delete request;
+    request->~RequestType();
    return nullptr;
  }

@ -170,7 +172,7 @@ class ServerStreamingHandler : public MethodHandler {
        return func_(service_, param.server_context,
                     static_cast<RequestType*>(param.request), &writer);
      });
-      delete static_cast<RequestType*>(param.request);
+      static_cast<RequestType*>(param.request)->~RequestType();
    }

    CallOpSet<CallOpSendInitialMetadata, CallOpServerSendStatus> ops;
@ -189,16 +191,18 @@ class ServerStreamingHandler : public MethodHandler {
    param.call->cq()->Pluck(&ops);
  }

-  void* Deserialize(grpc_byte_buffer* req, Status* status) final {
+  void* Deserialize(grpc_call* call, grpc_byte_buffer* req,
+                    Status* status) final {
    ByteBuffer buf;
    buf.set_buffer(req);
-    auto* request = new RequestType();
+    auto* request = new (g_core_codegen_interface->grpc_call_arena_alloc(
+        call, sizeof(RequestType))) RequestType();
    *status = SerializationTraits<RequestType>::Deserialize(&buf, request);
    buf.Release();
    if (status->ok()) {
      return request;
    }
-    delete request;
+    request->~RequestType();
    return nullptr;
  }

@ -323,7 +327,8 @@ class ErrorMethodHandler : public MethodHandler {
    param.call->cq()->Pluck(&ops);
  }

-  void* Deserialize(grpc_byte_buffer* req, Status* status) final {
+  void* Deserialize(grpc_call* call, grpc_byte_buffer* req,
+                    Status* status) final {
    // We have to destroy any request payload
    if (req != nullptr) {
      g_core_codegen_interface->grpc_byte_buffer_destroy(req);
--- a/include/grpcpp/impl/codegen/rpc_service_method.h
+++ b/include/grpcpp/impl/codegen/rpc_service_method.h
@ -56,7 +56,8 @@ class MethodHandler {
     a HandlerParameter and passed to RunHandler. It is illegal to access the
     pointer after calling RunHandler. Ownership of the deserialized request is
     retained by the handler. Returns nullptr if deserialization failed. */
-  virtual void* Deserialize(grpc_byte_buffer* req, Status* status) {
+  virtual void* Deserialize(grpc_call* call, grpc_byte_buffer* req,
+                            Status* status) {
    GPR_CODEGEN_ASSERT(req == nullptr);
    return nullptr;
  }
--- a/src/cpp/server/server_cc.cc
+++ b/src/cpp/server/server_cc.cc
@ -256,7 +256,8 @@ class Server::SyncRequest final : public internal::CompletionQueueTag {
        // Set interception point for RECV MESSAGE
        auto* handler = resources_ ? method_->handler()
                                   : server_->resource_exhausted_handler_.get();
-        request_ = handler->Deserialize(request_payload_, &request_status_);
+        request_ = handler->Deserialize(call_.call(), request_payload_,
+                                        &request_status_);

        request_payload_ = nullptr;
        interceptor_methods_.AddInterceptionHookPoint(
--- a/tools/run_tests/generated/sources_and_headers.json
+++ b/tools/run_tests/generated/sources_and_headers.json
@ -4720,6 +4720,28 @@
    "third_party": false, 
    "type": "target"
  }, 
+  {
+    "deps": [
+      "gpr", 
+      "gpr_test_util", 
+      "grpc", 
+      "grpc++", 
+      "grpc++_test_util", 
+      "grpc_test_util"
+    ], 
+    "headers": [
+      "test/cpp/end2end/interceptors_util.h"
+    ], 
+    "is_filegroup": false, 
+    "language": "c++", 
+    "name": "server_interceptors_end2end_test", 
+    "src": [
+      "test/cpp/end2end/interceptors_util.h", 
+      "test/cpp/end2end/server_interceptors_end2end_test.cc"
+    ], 
+    "third_party": false, 
+    "type": "target"
+  }, 
  {
    "deps": [
      "gpr", 
--- a/tools/run_tests/generated/tests.json
+++ b/tools/run_tests/generated/tests.json
@ -5141,6 +5141,30 @@
    ], 
    "uses_polling": true
  }, 
+  {
+    "args": [], 
+    "benchmark": false, 
+    "ci_platforms": [
+      "linux", 
+      "mac", 
+      "posix", 
+      "windows"
+    ], 
+    "cpu_cost": 0.5, 
+    "exclude_configs": [], 
+    "exclude_iomgrs": [], 
+    "flaky": false, 
+    "gtest": true, 
+    "language": "c++", 
+    "name": "server_interceptors_end2end_test", 
+    "platforms": [
+      "linux", 
+      "mac", 
+      "posix", 
+      "windows"
+    ], 
+    "uses_polling": true
+  }, 
  {
    "args": [], 
    "benchmark": false,