fix grpc: fix server outbound middleware run order

commit_hash:0b0bbaa4088d098e56acf04b1e9bd77b895e3fdf

Author: kpavlov00

grpc/include/userver/ugrpc/server/impl/call_processor.hpp

@@ -84,7 +84,7 @@ class CallProcessor final {
     )
         : state_(std::move(params), CallTraits::kCallKind),
           responder_(state_, raw_responder),
-          context_(utils::impl::InternalTag{}, state_),
+          middleware_call_context_(utils::impl::InternalTag{}, state_),
           initial_request_(initial_request),
           service_(service),
           service_method_(service_method)
@@ -105,15 +105,15 @@ class CallProcessor final {
         // Don't keep the config snapshot for too long, especially for streaming RPCs.
         state_.config_snapshot.reset();
 
+        // Final response is the response sent to the client together with status in the final batch.
+        std::optional<Response> final_response;
+
         if (!Status().ok()) {
-            RunOnCallFinish();
+            RunOnCallFinish(final_response);
             impl::ReportFinish(responder_.FinishWithError(Status()), Status(), state_);
             return;
         }
 
-        // Final response is the response sent to the client together with status in the final batch.
-        std::optional<Response> final_response{};
-
         RunWithCatch([this, &final_response] {
             auto result = CallHandler();
             impl::UnpackResult(std::move(result), final_response, Status());
@@ -128,15 +128,12 @@ class CallProcessor final {
         }
 
         if (!Status().ok()) {
-            RunOnCallFinish();
+            RunOnCallFinish(final_response);
             impl::ReportFinish(responder_.FinishWithError(Status()), Status(), state_);
             return;
         }
 
-        if (final_response) {
-            RunPreSendMessage(*final_response);
-        }
-        RunOnCallFinish();
+        RunOnCallFinish(final_response);
 
         if (!Status().ok()) {
             impl::ReportFinish(responder_.FinishWithError(Status()), Status(), state_);
@@ -171,44 +168,38 @@ class CallProcessor final {
     void RunOnCallStart() {
         UASSERT(success_pre_hooks_count_ == 0);
         for (const auto& m : state_.middlewares) {
-            RunWithCatch([this, &m] { m->OnCallStart(context_); });
+            RunWithCatch([this, &m] { m->OnCallStart(middleware_call_context_); });
             if (!Status().ok()) {
                 return;
             }
             // On fail, we must call OnRpcFinish only for middlewares for which OnRpcStart has been called successfully.
             // So, we watch to count of these middlewares.
             ++success_pre_hooks_count_;
             if constexpr (std::is_base_of_v<google::protobuf::Message, InitialRequest>) {
-                RunWithCatch([this, &m] { m->PostRecvMessage(context_, initial_request_); });
+                RunWithCatch([this, &m] { m->PostRecvMessage(middleware_call_context_, initial_request_); });
                 if (!Status().ok()) {
                     return;
                 }
             }
         }
     }
 
-    void RunOnCallFinish() {
+    void RunOnCallFinish(std::optional<Response>& final_response) {
         const auto& mids = state_.middlewares;
         const auto rbegin = mids.rbegin() + (mids.size() - success_pre_hooks_count_);
         for (auto it = rbegin; it != mids.rend(); ++it) {
             const auto& middleware = *it;
-            // We must call all OnRpcFinish despite the failures. So, don't check the status.
-            RunWithCatch([this, &middleware] { middleware->OnCallFinish(context_, Status()); });
-        }
-    }
 
-    void RunPreSendMessage(Response& response) {
-        if constexpr (std::is_base_of_v<google::protobuf::Message, Response>) {
-            const auto& mids = state_.middlewares;
-            // We don't want to include a heavy boost header for reverse view.
-            // NOLINTNEXTLINE(modernize-loop-convert)
-            for (auto it = mids.rbegin(); it != mids.rend(); ++it) {
-                const auto& middleware = *it;
-                RunWithCatch([this, &response, &middleware] { middleware->PreSendMessage(context_, response); });
-                if (!Status().ok()) {
-                    return;
+            if constexpr (std::is_base_of_v<google::protobuf::Message, Response>) {
+                if (Status().ok() && final_response.has_value()) {
+                    RunWithCatch([this, &middleware, &final_response] {
+                        middleware->PreSendMessage(middleware_call_context_, *final_response);
+                    });
                 }
             }
+
+            // We must call all OnRpcFinish despite the failures. So, don't check the status.
+            RunWithCatch([this, &middleware] { middleware->OnCallFinish(middleware_call_context_, Status()); });
         }
     }
 
@@ -230,11 +221,11 @@ class CallProcessor final {
         }
     }
 
-    grpc::Status& Status() { return context_.GetStatus(utils::impl::InternalTag{}); }
+    grpc::Status& Status() { return middleware_call_context_.GetStatus(utils::impl::InternalTag{}); }
 
     CallState state_;
     Responder responder_;
-    MiddlewareCallContext context_;
+    MiddlewareCallContext middleware_call_context_;
     // Initial request is the request which is sent to the service together with RPC initiation.
     // Unary-request RPCs have an initial request, client-streaming RPCs don't.
     InitialRequest& initial_request_;

grpc/src/ugrpc/server/middlewares/base.cpp

@@ -11,22 +11,6 @@ USERVER_NAMESPACE_BEGIN
 
 namespace ugrpc::server {
 
-MiddlewareBase::MiddlewareBase() = default;
-
-MiddlewareBase::~MiddlewareBase() = default;
-
-void MiddlewareBase::OnCallStart(MiddlewareCallContext&) const {}
-
-void MiddlewareBase::OnCallFinish(MiddlewareCallContext& context, const grpc::Status& status) const {
-    if (!status.ok()) {
-        return context.SetError(grpc::Status{status});
-    }
-}
-
-void MiddlewareBase::PostRecvMessage(MiddlewareCallContext&, google::protobuf::Message&) const {}
-
-void MiddlewareBase::PreSendMessage(MiddlewareCallContext&, google::protobuf::Message&) const {}
-
 MiddlewareCallContext::MiddlewareCallContext(utils::impl::InternalTag, impl::CallState& state)
     : CallContextBase(utils::impl::InternalTag{}, state)
 {}
@@ -56,6 +40,18 @@ ugrpc::impl::RpcStatisticsScope& MiddlewareCallContext::GetStatistics(utils::imp
     return GetCallState(utils::impl::InternalTag{}).statistics_scope;
 }
 
+MiddlewareBase::MiddlewareBase() = default;
+
+MiddlewareBase::~MiddlewareBase() = default;
+
+void MiddlewareBase::OnCallStart(MiddlewareCallContext&) const {}
+
+void MiddlewareBase::PostRecvMessage(MiddlewareCallContext&, google::protobuf::Message&) const {}
+
+void MiddlewareBase::PreSendMessage(MiddlewareCallContext&, google::protobuf::Message&) const {}
+
+void MiddlewareBase::OnCallFinish(MiddlewareCallContext&, const grpc::Status&) const {}
+
 }  // namespace ugrpc::server
 
 USERVER_NAMESPACE_END

grpc/tests/server_middleware_hooks_unary_test.cpp

@@ -229,7 +229,7 @@ UTEST_P(ServerMiddlewareHooksUnaryTest, ApplyTheLastErrorStatus) {
 
     EXPECT_CALL(Middleware(1), OnCallStart).Times(1);
     EXPECT_CALL(Middleware(1), PostRecvMessage).Times(1);
-    EXPECT_CALL(Middleware(1), PreSendMessage).Times(1);
+    EXPECT_CALL(Middleware(1), PreSendMessage).Times(0);
     // OnCallStart of M1 is successfully => OnCallFinish must be called.
     EXPECT_CALL(Middleware(1), OnCallFinish).Times(1);