Controller.SetFailed sets status_code as well

src/brpc/builtin/bthreads_service.cpp

@@ -45,7 +45,6 @@ void BthreadsService::default_method(::google::protobuf::RpcController* cntl_bas
         if (*endptr == '\0' || *endptr == '/') {
             ::bthread::print_task(os, tid);
         } else {
-            cntl->http_response().set_status_code(HTTP_STATUS_NOT_FOUND);
             cntl->SetFailed(ENOMETHOD, "path=%s is not a bthread id",
                             constraint.c_str());
         }

src/brpc/builtin/hotspots_service.cpp

@@ -372,7 +372,6 @@ static void DisplayResult(Controller* cntl,
             if (use_html) {
                 resp.append("</pre></body></html>");
             }
-            cntl->http_response().set_status_code(HTTP_STATUS_OK);
             return;
         }
     }
@@ -481,7 +480,6 @@ static void DisplayResult(Controller* cntl,
     if (use_html) {
         resp.append("</pre></body></html>");
     }
-    cntl->http_response().set_status_code(HTTP_STATUS_OK);
 }
 
 static void DoProfiling(ProfilingType type,

src/brpc/builtin/ids_service.cpp

@@ -47,9 +47,9 @@ void IdsService::default_method(::google::protobuf::RpcController* cntl_base,
         if (*endptr == '\0' || *endptr == '/') {
             bthread::id_status(id, os);
         } else {
-            cntl->http_response().set_status_code(HTTP_STATUS_NOT_FOUND);
             cntl->SetFailed(ENOMETHOD, "path=%s is not a bthread_id",
                             constraint.c_str());
+            return;
         }
     }
     os.move_to(cntl->response_attachment());

src/brpc/builtin/sockets_service.cpp

@@ -43,7 +43,6 @@ void SocketsService::default_method(::google::protobuf::RpcController* cntl_base
         if (*endptr == '\0' || *endptr == '/') {
             Socket::DebugSocket(os, sid);
         } else {
-            cntl->http_response().set_status_code(HTTP_STATUS_NOT_FOUND);
             cntl->SetFailed(ENOMETHOD, "path=%s is not a SocketId",
                             constraint.c_str());
         }

src/brpc/builtin/vars_service.cpp

@@ -318,11 +318,9 @@ void VarsService::default_method(::google::protobuf::RpcController* cntl_base,
             cntl->http_response().set_content_type("application/json");
             os.move_to(cntl->response_attachment());
         } else if (rc < 0) {
-            cntl->http_response().set_status_code(HTTP_STATUS_NOT_FOUND);
             cntl->SetFailed(ENOMETHOD, "Fail to find any bvar by `%s'",
                             cntl->http_request().unresolved_path().c_str());
         } else {
-            cntl->http_response().set_status_code(HTTP_STATUS_NOT_FOUND);
             cntl->SetFailed(ENODATA, "`%s' does not have value series",
                             cntl->http_request().unresolved_path().c_str());
         }
@@ -419,7 +417,6 @@ void VarsService::default_method(::google::protobuf::RpcController* cntl_base,
         return;
     }
     if (!options.white_wildcards.empty() && ndump == 0) {
-        cntl->http_response().set_status_code(HTTP_STATUS_NOT_FOUND);
         cntl->SetFailed(ENOMETHOD, "Fail to find any bvar by `%s'",
                         options.white_wildcards.c_str());
     }

src/brpc/channel.cpp

@@ -251,6 +251,9 @@ void Channel::CallMethod(const google::protobuf::MethodDescriptor* method,
         // in correlation_id. negative max_retry causes undefined behavior.
         cntl->set_max_retry(0);
     }
+    // HTTP needs this field to be set before any SetFailed()
+    cntl->_request_protocol = _options.protocol;
+    cntl->_preferred_index = _preferred_index;
     cntl->_retry_policy = _options.retry_policy;
     const CallId correlation_id = cntl->call_id();
     const int rc = bthread_id_lock_and_reset_range(
@@ -321,8 +324,6 @@ void Channel::CallMethod(const google::protobuf::MethodDescriptor* method,
         cntl->_single_server_id = _server_id;
         cntl->_remote_side = _server_address;
     }
-    cntl->_request_protocol = _options.protocol;
-    cntl->_preferred_index = _preferred_index;
 
     // Share the lb with controller.
     cntl->_lb = _lb;

src/brpc/controller.cpp

@@ -355,6 +355,30 @@ void Controller::AppendServerIdentiy() {
     }
 }
 
+// Defined in http_rpc_protocol.cpp
+namespace policy {
+int ErrorCode2StatusCode(int error_code);
+}
+
+inline void UpdateResponseHeader(Controller* cntl) {
+    DCHECK(cntl->Failed());
+    if (cntl->request_protocol() == PROTOCOL_HTTP) {
+        if (cntl->ErrorCode() != EHTTP) {
+            // We assume that status code is already set along with EHTTP.
+            cntl->http_response().set_status_code(
+                policy::ErrorCode2StatusCode(cntl->ErrorCode()));
+        }
+        if (cntl->server() != NULL) {
+            // Override HTTP body at server-side to conduct error text
+            // to the client.
+            // The client-side should preserve body which may be a piece
+            // of useable data rather than error text.
+            cntl->response_attachment().clear();
+            cntl->response_attachment().append(cntl->ErrorText());
+        }
+    }
+}
+
 void Controller::SetFailed(const std::string& reason) {
     _error_code = -1;
     if (!_error_text.empty()) {
@@ -370,6 +394,7 @@ void Controller::SetFailed(const std::string& reason) {
         _span->set_error_code(_error_code);
         _span->Annotate(reason);
     }
+    UpdateResponseHeader(this);
 }
 
 void Controller::SetFailed(int error_code, const char* reason_fmt, ...) {
@@ -398,6 +423,7 @@ void Controller::SetFailed(int error_code, const char* reason_fmt, ...) {
         _span->set_error_code(_error_code);
         _span->AnnotateCStr(_error_text.c_str() + old_size, 0);
     }
+    UpdateResponseHeader(this);
 }
 
 void Controller::CloseConnection(const char* reason_fmt, ...) {
@@ -425,6 +451,7 @@ void Controller::CloseConnection(const char* reason_fmt, ...) {
         _span->set_error_code(_error_code);
         _span->AnnotateCStr(_error_text.c_str() + old_size, 0);
     }
+    UpdateResponseHeader(this);
 }
 
 bool Controller::IsCanceled() const {

src/brpc/controller.h

@@ -333,7 +333,7 @@ public:
     // Tell RPC to close the connection instead of sending back response.
     // If this controller was not SetFailed() before, ErrorCode() will be
     // set to ECLOSE.
-    // Notice that the actual closing does not take place immediately.
+    // NOTE: the underlying connection is not closed immediately.
     void CloseConnection(const char* reason_fmt, ...);
 
     // True if CloseConnection() was called.
@@ -384,6 +384,10 @@ public:
     
     // Causes Failed() to return true on the client side.  "reason" will be
     // incorporated into the message returned by ErrorText().
+    // NOTE: Change http_response().status_code() according to `error_code'
+    // as well if the protocol is HTTP. If you want to overwrite the 
+    // status_code, call http_response().set_status_code() after SetFailed()
+    // (rather than before SetFailed)
     void SetFailed(const std::string& reason);
     void SetFailed(int error_code, const char* reason_fmt, ...)
         __attribute__ ((__format__ (__printf__, 3, 4)));

src/brpc/details/http_message.cpp

@@ -116,10 +116,11 @@ int HttpMessage::on_header_value(http_parser *parser,
                     << http_message->_url << " HTTP/" << parser->http_major
                     << '.' << parser->http_minor;
             } else {
+                // NOTE: http_message->header().status_code() may not be set yet.
                 *vs << "[HTTP RESPONSE @" << butil::my_ip() << "]\n< HTTP/"
                     << parser->http_major
                     << '.' << parser->http_minor << ' ' << parser->status_code
-                    << ' ' << http_message->header().reason_phrase();
+                    << ' ' << HttpReasonPhrase(parser->status_code);
             }
         }
         if (first_entry) {
@@ -215,7 +216,11 @@ int HttpMessage::OnBody(const char *at, const size_t length) {
             // only add prefix at first entry.
             *_vmsgbuilder << "\n<\n";
         }
-        if (_read_body_progressively) {
+        if (_read_body_progressively &&
+            // If the status_code is non-OK, the body is likely to be the error
+            // description which is very helpful for debugging. Otherwise
+            // the body is probably streaming data which is too long to print.
+            header().status_code() == HTTP_STATUS_OK) {
             std::cerr << _vmsgbuilder->buf() << std::endl;
             delete _vmsgbuilder;
             _vmsgbuilder = NULL;

src/brpc/policy/http_rpc_protocol.cpp

@@ -264,11 +264,20 @@ void ProcessHttpResponse(InputMessageBase* msg) {
             accessor.set_readable_progressive_attachment(imsg_guard.get());
             const int sc = res_header->status_code();
             if (sc < 200 || sc >= 300) {
-                cntl->SetFailed(EHTTP, "HTTP/%d.%d %d %s",
+                // Even if the body is for streaming purpose, a non-OK status
+                // code indicates that the body is probably the error text
+                // which is helpful for debugging.
+                // content may be binary data, so the size limit is a must.
+                std::string body_str;
+                res_body.copy_to(
+                    &body_str, std::min((int)res_body.size(),
+                                        FLAGS_http_max_error_length));
+                cntl->SetFailed(EHTTP, "HTTP/%d.%d %d %s: %.*s",
                                 res_header->major_version(),
                                 res_header->minor_version(),
                                 static_cast<int>(res_header->status_code()),
-                                res_header->reason_phrase());
+                                res_header->reason_phrase(),
+                                (int)body_str.size(), body_str.c_str());
             } else if (cntl->response() != NULL &&
                        cntl->response()->GetDescriptor()->field_count() != 0) {
                 cntl->SetFailed(ERESPONSE, "A protobuf response can't be parsed"
@@ -284,7 +293,6 @@ void ProcessHttpResponse(InputMessageBase* msg) {
             if (!res_body.empty()) {
                 // Use content as error text if it's present. Notice that
                 // content may be binary data, so the size limit is a must.
-                // TODO: Print body in better way.
                 std::string body_str;
                 res_body.copy_to(
                     &body_str, std::min((int)res_body.size(),
@@ -363,29 +371,19 @@ void ProcessHttpResponse(InputMessageBase* msg) {
     accessor.OnResponse(cid, saved_error);
 }
 
-void UpdateResponseHeader(int status_code, Controller* cntl) {
-    DCHECK(cntl->Failed());
-    HttpHeader* resp_header = &cntl->http_response();
-    resp_header->set_status_code(status_code);
-    cntl->response_attachment().clear();
-    cntl->response_attachment().append(cntl->ErrorText());
-}
-
 void SerializeHttpRequest(butil::IOBuf* /*not used*/,
                           Controller* cntl,
                           const google::protobuf::Message* request) {
     if (request != NULL) {
         // If request is not NULL, message body will be serialized json,
         if (!request->IsInitialized()) {
-            cntl->SetFailed(
+            return cntl->SetFailed(
                 EREQUEST, "Missing required fields in request: %s",
                 request->InitializationErrorString().c_str());
-            return UpdateResponseHeader(HTTP_STATUS_BAD_REQUEST, cntl);
         }
         if (!cntl->request_attachment().empty()) {
-            cntl->SetFailed(EREQUEST, "request_attachment must be empty "
-                            "when request is not NULL");
-            return UpdateResponseHeader(HTTP_STATUS_BAD_REQUEST, cntl);
+            return cntl->SetFailed(EREQUEST, "request_attachment must be empty "
+                                   "when request is not NULL");
         }
         butil::IOBufAsZeroCopyOutputStream wrapper(&cntl->request_attachment());
         const HttpContentType content_type
@@ -394,10 +392,8 @@ void SerializeHttpRequest(butil::IOBuf* /*not used*/,
             // Serialize content as protobuf
             if (!request->SerializeToZeroCopyStream(&wrapper)) {
                 cntl->request_attachment().clear();
-                cntl->SetFailed(EREQUEST, "Fail to serialize %s",
-                                request->GetTypeName().c_str());
-                UpdateResponseHeader(HTTP_STATUS_BAD_REQUEST, cntl);
-                return;
+                return cntl->SetFailed(EREQUEST, "Fail to serialize %s",
+                                       request->GetTypeName().c_str());
             }
         } else {
             // Serialize content as json
@@ -409,10 +405,7 @@ void SerializeHttpRequest(butil::IOBuf* /*not used*/,
                                : json2pb::OUTPUT_ENUM_BY_NAME);
             if (!json2pb::ProtoMessageToJson(*request, &wrapper, opt, &err)) {
                 cntl->request_attachment().clear();
-                cntl->SetFailed(EREQUEST, "Fail to convert request to json, %s",
-                                err.c_str());
-                UpdateResponseHeader(HTTP_STATUS_BAD_REQUEST, cntl);
-                return;
+                return cntl->SetFailed(EREQUEST, "Fail to convert request to json, %s", err.c_str());
             }
             // Set content-type if user did not.
             if (cntl->http_request().content_type().empty()) {
@@ -425,15 +418,13 @@ void SerializeHttpRequest(butil::IOBuf* /*not used*/,
     }
     // Make RPC fail if uri() is not OK (previous SetHttpURL/operator= failed)
     if (!cntl->http_request().uri().status().ok()) {
-        cntl->SetFailed(EREQUEST, "%s",
+        return cntl->SetFailed(EREQUEST, "%s",
                         cntl->http_request().uri().status().error_cstr());
-        return UpdateResponseHeader(HTTP_STATUS_BAD_REQUEST, cntl);
     }
     if (cntl->request_compress_type() != COMPRESS_TYPE_NONE) {
         if (cntl->request_compress_type() != COMPRESS_TYPE_GZIP) {
-            cntl->SetFailed(EREQUEST, "http does not support %s",
+            return cntl->SetFailed(EREQUEST, "http does not support %s",
                             CompressTypeToCStr(cntl->request_compress_type()));
-            return UpdateResponseHeader(HTTP_STATUS_BAD_REQUEST, cntl);
         }
         const size_t request_size = cntl->request_attachment().size();
         if (request_size >= (size_t)FLAGS_http_body_compress_threshold) {
@@ -499,17 +490,14 @@ void PackHttpRequest(butil::IOBuf* buf,
                      const butil::IOBuf& /*unused*/,
                      const Authenticator* auth) {
     if (cntl->connection_type() == CONNECTION_TYPE_SINGLE) {
-        cntl->SetFailed(EREQUEST, "http can't work with CONNECTION_TYPE_SINGLE");
-        return UpdateResponseHeader(HTTP_STATUS_BAD_REQUEST, cntl);
+        return cntl->SetFailed(EREQUEST, "http can't work with CONNECTION_TYPE_SINGLE");
     }
     ControllerPrivateAccessor accessor(cntl);
     HttpHeader* header = &cntl->http_request();
     if (auth != NULL && header->GetHeader(common->AUTHORIZATION) == NULL) {
         std::string auth_data;
         if (auth->GenerateCredential(&auth_data) != 0) {
-            cntl->SetFailed(EREQUEST, "Fail to GenerateCredential");
-            UpdateResponseHeader(HTTP_STATUS_BAD_REQUEST, cntl);
-            return;
+            return cntl->SetFailed(EREQUEST, "Fail to GenerateCredential");
         }
         header->SetHeader(common->AUTHORIZATION, auth_data);
     }
@@ -534,7 +522,8 @@ inline bool SupportGzip(Controller* cntl) {
     return encodings->find(common->GZIP) != std::string::npos;
 }
 
-inline int ErrorCode2StatusCode(int error_code) {
+// Called in controller.cpp as well
+int ErrorCode2StatusCode(int error_code) {
     switch (error_code) {
     case ENOSERVICE:
     case ENOMETHOD:
@@ -609,8 +598,7 @@ static void SendHttpResponse(Controller *cntl,
                     res_header->set_content_type(common->CONTENT_TYPE_PROTO);
                 }
             } else {
-                cntl->SetFailed(ERESPONSE, "Fail to serialize %s",
-                                res->GetTypeName().c_str());
+                cntl->SetFailed(ERESPONSE, "Fail to serialize %s", res->GetTypeName().c_str());
             }
         } else {
             std::string err;
@@ -625,8 +613,7 @@ static void SendHttpResponse(Controller *cntl,
                     res_header->set_content_type(common->CONTENT_TYPE_JSON);
                 }
             } else {
-                cntl->SetFailed(ERESPONSE, "Fail to convert response to json, %s",
-                                err.c_str());
+                cntl->SetFailed(ERESPONSE, "Fail to convert response to json, %s", err.c_str());
             }
         }
     }
@@ -733,8 +720,7 @@ static void SendHttpResponse(Controller *cntl,
         // EPIPE is common in pooled connections + backup requests.
         const int errcode = errno;
         PLOG_IF(WARNING, errcode != EPIPE) << "Fail to write into " << *socket;
-        cntl->SetFailed(errcode, "Fail to write into %s",
-                        socket->description().c_str());
+        cntl->SetFailed(errcode, "Fail to write into %s", socket->description().c_str());
         return;
     }
     if (span) {
@@ -1131,8 +1117,6 @@ void ProcessHttpRequest(InputMessageBase *msg) {
     
     if (!server->IsRunning()) {
         cntl->SetFailed(ELOGOFF, "Server is stopping");
-        cntl->http_response().set_status_code(HTTP_STATUS_FORBIDDEN);
-        cntl->http_response().SetHeader(common->CONNECTION, common->CLOSE);
         return SendHttpResponse(cntl.release(), server, NULL);
     }
 
@@ -1172,7 +1156,6 @@ void ProcessHttpRequest(InputMessageBase *msg) {
         } else {
             cntl->SetFailed(ENOMETHOD, "Fail to find method on `%s'", path.c_str());
         }
-        cntl->http_response().set_status_code(HTTP_STATUS_NOT_FOUND);
         return SendHttpResponse(cntl.release(), server, NULL);
     } else if (sp->service->GetDescriptor() == BadMethodService::descriptor()) {
         BadMethodRequest breq;
@@ -1214,7 +1197,6 @@ void ProcessHttpRequest(InputMessageBase *msg) {
         cntl->SetFailed(EPERM, "Not allowed to access builtin services, try "
                         "ServerOptions.internal_port=%d instead if you're in"
                         " internal network", server->options().internal_port);
-        cntl->http_response().set_status_code(HTTP_STATUS_FORBIDDEN);
         return SendHttpResponse(cntl.release(), server, method_status);
     }
     
@@ -1242,7 +1224,6 @@ void ProcessHttpRequest(InputMessageBase *msg) {
                 cntl->SetFailed(EREQUEST, "%s needs to be created from a"
                                 " non-empty json, it has required fields.",
                                 req->GetDescriptor()->full_name().c_str());
-                cntl->http_response().set_status_code(HTTP_STATUS_BAD_REQUEST);
                 return SendHttpResponse(cntl.release(), server, method_status);
             } // else all fields of the request are optional.
         } else {

test/brpc_http_rpc_protocol_unittest.cpp

@@ -277,7 +277,7 @@ TEST_F(HttpTest, process_request_logoff) {
     _server._status = brpc::Server::READY;
     ProcessMessage(brpc::policy::ProcessHttpRequest, msg, false);
     ASSERT_EQ(1ll, _server._nerror.get_value());
-    CheckResponseCode(false, brpc::HTTP_STATUS_FORBIDDEN);
+    CheckResponseCode(false, brpc::HTTP_STATUS_SERVICE_UNAVAILABLE);
 }
 
 TEST_F(HttpTest, process_request_wrong_method) {
@@ -570,8 +570,12 @@ TEST_F(HttpTest, read_failed_chunked_response) {
     cntl.http_request().uri() = "/DownloadService/DownloadFailed";
     cntl.response_will_be_read_progressively();
     channel.CallMethod(NULL, &cntl, NULL, NULL, NULL);
-    ASSERT_TRUE(cntl.response_attachment().empty());
-    ASSERT_TRUE(cntl.Failed()) << cntl.ErrorText();
+    EXPECT_TRUE(cntl.response_attachment().empty());
+    ASSERT_TRUE(cntl.Failed());
+    ASSERT_NE(cntl.ErrorText().find("HTTP/1.1 500 Internal Server Error"),
+              std::string::npos) << cntl.ErrorText();
+    ASSERT_NE(cntl.ErrorText().find("Intentionally set controller failed"),
+              std::string::npos) << cntl.ErrorText();
     ASSERT_EQ(0, svc.last_errno());
 }
 

test/brpc_server_unittest.cpp

@@ -486,6 +486,7 @@ TEST_F(ServerTest, missing_required_fields) {
     http_channel.CallMethod(NULL, &cntl, NULL, NULL, NULL);
     ASSERT_TRUE(cntl.Failed());
     ASSERT_EQ(brpc::EHTTP, cntl.ErrorCode());
+    LOG(INFO) << cntl.ErrorText();
     ASSERT_EQ(brpc::HTTP_STATUS_BAD_REQUEST, cntl.http_response().status_code());
     ASSERT_EQ(0, service_v1.ncalled.load());
 

tools/patch_from_baidu

 #!/bin/bash
 if [ -z "$1" ]; then
-    echo "Usage1: patch_from_svn PATCHFILE (generated by 'svn diff -c REVISION')"
-    echo "Usage2: patch_from_svn SVN_DIR REVISION"
+    echo "Usage1: patch_from_baidu PATCHFILE (generated by 'git diff --no-prefix')"
+    echo "Usage2: patch_from_baidu GIT_DIR COMMIT"
     exit 1
 fi
 PATCHFILE=$1
-if [ -d "$1/.svn" ]; then
+if [ -d "$1/.git" ]; then
     if [ -z "$2" ]; then
-        echo "Second argument must be a SVN revision"
+        echo "Second argument must be a git commit"
         exit 1
     fi
     CURRENT_DIR=`pwd`
-    SVN_DIR=$1
-    REV=$2
-    PATCHFILE=$CURRENT_DIR/$REV.patch
-    echo "*** Generating diff of $SVN_DIR@$REV"
-    cd $SVN_DIR && svn diff . -c $REV > $PATCHFILE
+    GIT_DIR=$1
+    COMMIT=$2
+    PATCHFILE=$CURRENT_DIR/$COMMIT.patch
+    echo "*** Generating diff of $GIT_DIR@$COMMIT"
+    cd $GIT_DIR && git diff --no-prefix $COMMIT^! > $PATCHFILE
     cd $CURRENT_DIR
 fi
 
-MODIFIED_PATCHFILE=$(basename $(basename $PATCHFILE .patch) .diff).from_svn.patch
+MODIFIED_PATCHFILE=$(basename $(basename $PATCHFILE .patch) .diff).from_baidu.patch
 
 # guess prefix of test files
 TEST_PREFIX="test_"
 TEST_SUFFIX=
-if fgrep -q " bthread/" $PATCHFILE; then
+if fgrep -q " src/baidu/rpc/" $PATCHFILE; then
+    TEST_PREFIX="brpc_"
+    TEST_SUFFIX="_unittest"
+elif fgrep -q " bthread/" $PATCHFILE; then
     TEST_PREFIX="bthread_"
     TEST_SUFFIX="_unittest"
 elif fgrep -q " bvar/" $PATCHFILE; then
@@ -35,7 +38,6 @@ fi
 cat $PATCHFILE | sed -e 's/src\/baidu\/rpc\//src\/brpc\//g' \
             -e 's/\<baidu\/rpc\//brpc\//g' \
             -e 's/\<baidu\-rpc\([^-]\)/brpc\1/g' \
-            -e 's/\<src\/brpc\/test\/test_\(\S*\)\.cpp/test\/brpc_\1_unittest.cpp/g' \
             -e 's/\<test\/test_bthread\.cpp/test\/bthread_unittest.cpp/g' \
             -e 's/\<test\/test_object_pool\.cpp/test\/object_pool_unittest.cpp/g' \
             -e 's/\<test\/test_resource_pool\.cpp/test\/resource_pool_unittest.cpp/g' \
@@ -70,7 +72,7 @@ cat $PATCHFILE | sed -e 's/src\/baidu\/rpc\//src\/brpc\//g' \
 EXTRA_ARGS=
 if [ -z "$DO_RUN" ]; then
     EXTRA_ARGS="--dry-run $EXTRA_ARGS"
-    echo "*** This is a dry-run. To really apply, run: DO_RUN=1 tools/patch_from_svn $1"
+    echo "*** This is a dry-run. To really apply, run: DO_RUN=1 tools/patch_from_baidu $1"
 fi
 patch -p0 -u -l $EXTRA_ARGS < $MODIFIED_PATCHFILE
 if [ ! -z "$DO_RUN" ]; then