Not pass errors to ProcessThriftFramedRequestNoExcept

src/brpc/policy/thrift_protocol.cpp

@@ -449,6 +449,7 @@ void ProcessThriftRequest(InputMessageBase* msg_base) {
     ScopedNonServiceError non_service_error(server);
 
     ThriftClosure* thrift_done = new ThriftClosure;
+    ClosureGuard done_guard(thrift_done);
     Controller* cntl = &(thrift_done->_controller);
     ThriftFramedMessage* req = &(thrift_done->_request);
     ThriftFramedMessage* res = &(thrift_done->_response);
@@ -471,8 +472,7 @@ void ProcessThriftRequest(InputMessageBase* msg_base) {
     butil::Status st = ReadThriftMessageBegin(
         &msg->payload, &cntl->_thrift_method_name, &mtype, &seq_id);
     if (!st.ok()) {
-        cntl->SetFailed(EREQUEST, "%s", st.error_cstr());
-        return thrift_done->Run();
+        return cntl->SetFailed(EREQUEST, "%s", st.error_cstr());
     }
     msg->payload.swap(req->body);
     req->field_id = THRIFT_REQUEST_FID;
@@ -483,8 +483,7 @@ void ProcessThriftRequest(InputMessageBase* msg_base) {
         LOG_EVERY_SECOND(ERROR)
             << "Received thrift request however the server does not set"
             " ServerOptions.thrift_service, close the connection.";
-        cntl->SetFailed(EINTERNAL, "ServerOptions.thrift_service is NULL");
-        return thrift_done->Run();
+        return cntl->SetFailed(EINTERNAL, "ServerOptions.thrift_service is NULL");
     }
 
     // Switch to service-specific error.
@@ -492,10 +491,9 @@ void ProcessThriftRequest(InputMessageBase* msg_base) {
     MethodStatus* method_status = service->_status;
     if (method_status) {
         if (!method_status->OnRequested()) {
-            cntl->SetFailed(ELIMIT, "Reached %s's max_concurrency=%d",
+            return cntl->SetFailed(ELIMIT, "Reached %s's max_concurrency=%d",
                             cntl->thrift_method_name().c_str(),
                             method_status->MaxConcurrency());
-            return thrift_done->Run();
         }
     }
 
@@ -516,27 +514,21 @@ void ProcessThriftRequest(InputMessageBase* msg_base) {
         span->set_request_size(sizeof(thrift_head_t) + req->body.size());
     }
 
-    do {
-        if (!server->IsRunning()) {
-            cntl->SetFailed(ELOGOFF, "Server is stopping");
-            break;
-        }
-        if (socket->is_overcrowded()) {
-            cntl->SetFailed(EOVERCROWDED, "Connection to %s is overcrowded",
-                            butil::endpoint2str(socket->remote_side()).c_str());
-            break;
-        }
-        if (!server_accessor.AddConcurrency(cntl)) {
-            cntl->SetFailed(ELIMIT, "Reached server's max_concurrency=%d",
-                            server->options().max_concurrency);
-            break;
-        }
-        if (FLAGS_usercode_in_pthread && TooManyUserCode()) {
-            cntl->SetFailed(ELIMIT, "Too many user code to run when"
-                            " -usercode_in_pthread is on");
-            break;
-        }
-    } while (false);
+    if (!server->IsRunning()) {
+        return cntl->SetFailed(ELOGOFF, "Server is stopping");
+    }
+    if (socket->is_overcrowded()) {
+        return cntl->SetFailed(EOVERCROWDED, "Connection to %s is overcrowded",
+                butil::endpoint2str(socket->remote_side()).c_str());
+    }
+    if (!server_accessor.AddConcurrency(cntl)) {
+        return cntl->SetFailed(ELIMIT, "Reached server's max_concurrency=%d",
+                server->options().max_concurrency);
+    }
+    if (FLAGS_usercode_in_pthread && TooManyUserCode()) {
+        return cntl->SetFailed(ELIMIT, "Too many user code to run when"
+                " -usercode_in_pthread is on");
+    }
 
     msg.reset();  // optional, just release resourse ASAP
 
@@ -546,6 +538,8 @@ void ProcessThriftRequest(InputMessageBase* msg_base) {
         span->AsParent();
     }
 
+    done_guard.release();
+
     if (!FLAGS_usercode_in_pthread) {
         return ProcessThriftFramedRequestNoExcept(service, cntl, req, res, thrift_done);
     }

src/brpc/thrift_service.h

@@ -38,12 +38,9 @@ public:
     ThriftService();
     virtual ~ThriftService();
 
-    // Implement this method to handle thrift_binary requests. Notice that this
-    // method can be called with a failed Controller(something wrong with the
-    // request before calling this method), in which case the implemenetation
-    // shall send specific response with error information back to client.
+    // Implement this method to handle thrift_binary requests.
     // Parameters:
-    //   controller  per-rpc settings.
+    //   controller  per-rpc settings. controller->Failed() is always false.
     //   request     The thrift_binary request received.
     //   response    The thrift_binary response that you should fill in.
     //   done        You must call done->Run() to end the processing.