Merge from svn r35496 r35493, solving the ENOMEM issue of profiling

Makefile

@@ -139,7 +139,8 @@ BUTIL_SOURCES = \
     src/butil/zero_copy_stream_as_streambuf.cpp \
     src/butil/crc32c.cc \
     src/butil/containers/case_ignored_flat_map.cpp \
-    src/butil/iobuf.cpp
+    src/butil/iobuf.cpp \
+    src/butil/popen.cpp
 
 BUTIL_OBJS = $(addsuffix .o, $(basename $(BUTIL_SOURCES)))
 

src/brpc/builtin/hotspots_service.cpp

@@ -18,6 +18,7 @@
 #include <gflags/gflags.h>
 #include "butil/files/file_enumerator.h"
 #include "butil/file_util.h"                     // butil::FilePath
+#include "butil/popen.h"                        // butil::read_command_output
 #include "brpc/log.h"
 #include "brpc/controller.h"
 #include "brpc/server.h"
@@ -360,7 +361,7 @@ static void DisplayResult(Controller* cntl,
                 // retry;
             }
         }
-        CHECK_EQ(0, fclose(fp));
+        PLOG_IF(ERROR, fclose(fp) != 0) << "Fail to close fp";
         if (succ) {
             RPC_VLOG << "Hit cache=" << expected_result_name;
             os.move_to(resp);
@@ -403,35 +404,11 @@ static void DisplayResult(Controller* cntl,
             }
             g_written_pprof_perl = true;
         }
-        errno = 0; // popen may not set errno, clear it to make sure if
+        errno = 0; // read_command_output may not set errno, clear it to make sure if
                    // we see non-zero errno, it's real error.
-        FILE* pipe = popen(cmd.c_str(), "r");
-        if (pipe == NULL) {
-            os << "Fail to popen `" << cmd << "', " << berror()
-               << (use_html ? "</body></html>" : "\n");
-            os.move_to(resp);
-            cntl->http_response().set_status_code(
-                HTTP_STATUS_INTERNAL_SERVER_ERROR);
-            return;
-        }
-        char buffer[1024];
-        while (1) {
-            size_t nr = fread(buffer, 1, sizeof(buffer), pipe);
-            if (nr != 0) {
-                prof_result.append(buffer, nr);
-            }
-            if (nr != sizeof(buffer)) {
-                if (feof(pipe)) {
-                    break;
-                } else if (ferror(pipe)) {
-                    LOG(ERROR) << "Encountered error while reading for the pipe";
-                    break;
-                }
-                // retry;
-            }
-        }
-        if (pclose(pipe) != 0) {
-            // NOTE: pclose may fail if the command failed to run, quit normal.
+        butil::IOBufBuilder pprof_output;
+        const int rc = butil::read_command_output(pprof_output, cmd.c_str());
+        if (rc != 0) {
             butil::FilePath path(pprof_tool);
             if (!butil::PathExists(path)) {
                 // Write the script again.
@@ -440,48 +417,57 @@ static void DisplayResult(Controller* cntl,
                 os << path.value() << " was removed, recreate ...\n\n";
                 continue;
             }
-        } else {
-            // Cache result in file.
-            char result_name[256];
-            MakeCacheName(result_name, sizeof(result_name), prof_name,
-                          GetBaseName(base_name), use_text, show_ccount);
-
-            // Append the profile name as the visual reminder for what
-            // current profile is.
-            butil::IOBuf before_label;
-            butil::IOBuf tmp;
-            if (cntl->http_request().uri().GetQuery("view") == NULL) {
-                tmp.append(prof_name);
-                tmp.append("[addToProfEnd]");
+            if (rc < 0) {
+                os << "Fail to execute `" << cmd << "', " << berror()
+                   << (use_html ? "</body></html>" : "\n");
+                os.move_to(resp);
+                cntl->http_response().set_status_code(
+                    HTTP_STATUS_INTERNAL_SERVER_ERROR);
+                return;
             }
-            if (prof_result.cut_until(&before_label, ",label=\"") == 0) {
-                tmp.append(before_label);
-                tmp.append(",label=\"[");
-                tmp.append(GetBaseName(prof_name));
-                if (base_name) {
-                    tmp.append(" - ");
-                    tmp.append(GetBaseName(base_name));
-                }
-                tmp.append("]\\l");
-                tmp.append(prof_result);
-                tmp.swap(prof_result);
-            } else {
-                // Assume it's text. append before result directly.
-                tmp.append("[");
-                tmp.append(GetBaseName(prof_name));
-                if (base_name) {
-                    tmp.append(" - ");
-                    tmp.append(GetBaseName(base_name));
-                }
-                tmp.append("]\n");
-                tmp.append(prof_result);
-                tmp.swap(prof_result);
+            // cmd returns non zero, quit normally 
+        }
+        pprof_output.move_to(prof_result);
+        // Cache result in file.
+        char result_name[256];
+        MakeCacheName(result_name, sizeof(result_name), prof_name,
+                      GetBaseName(base_name), use_text, show_ccount);
+
+        // Append the profile name as the visual reminder for what
+        // current profile is.
+        butil::IOBuf before_label;
+        butil::IOBuf tmp;
+        if (cntl->http_request().uri().GetQuery("view") == NULL) {
+            tmp.append(prof_name);
+            tmp.append("[addToProfEnd]");
+        }
+        if (prof_result.cut_until(&before_label, ",label=\"") == 0) {
+            tmp.append(before_label);
+            tmp.append(",label=\"[");
+            tmp.append(GetBaseName(prof_name));
+            if (base_name) {
+                tmp.append(" - ");
+                tmp.append(GetBaseName(base_name));
             }
-            
-            if (!WriteSmallFile(result_name, prof_result)) {
-                LOG(ERROR) << "Fail to write " << result_name;
-                CHECK(butil::DeleteFile(butil::FilePath(result_name), false));
+            tmp.append("]\\l");
+            tmp.append(prof_result);
+            tmp.swap(prof_result);
+        } else {
+            // Assume it's text. append before result directly.
+            tmp.append("[");
+            tmp.append(GetBaseName(prof_name));
+            if (base_name) {
+                tmp.append(" - ");
+                tmp.append(GetBaseName(base_name));
             }
+            tmp.append("]\n");
+            tmp.append(prof_result);
+            tmp.swap(prof_result);
+        }
+        
+        if (!WriteSmallFile(result_name, prof_result)) {
+            LOG(ERROR) << "Fail to write " << result_name;
+            CHECK(butil::DeleteFile(butil::FilePath(result_name), false));
         }
         break;
     }
@@ -863,6 +849,10 @@ static void StartProfiling(ProfilingType type,
     os <<
         "    var index = data.indexOf('digraph ');\n"
         "    if (index == -1) {\n"
+        "      var selEnd = data.indexOf('[addToProfEnd]');\n"
+        "      if (selEnd != -1) {\n"
+        "        data = data.substring(selEnd + '[addToProfEnd]'.length);\n"
+        "      }\n"
         "      $(\"#profiling-result\").html('<pre>' + data + '</pre>');\n"
         "    } else {\n"
         "      $(\"#profiling-result\").html('Plotting ...');\n"

src/brpc/builtin/pprof_service.cpp

@@ -23,6 +23,7 @@
 #include "butil/file_util.h"                 // butil::FilePath
 #include "butil/files/scoped_file.h"         // ScopedFILE
 #include "butil/time.h"
+#include "butil/popen.h"                    // butil::read_command_output
 #include "brpc/log.h"
 #include "brpc/controller.h"                // Controller
 #include "brpc/closure_guard.h"             // ClosureGuard
@@ -289,16 +290,15 @@ static int ExtractSymbolsFromBinary(
     tm.start();
     std::string cmd = "nm -C -p ";
     cmd.append(lib_info.path);
-    FILE* pipe = popen(cmd.c_str(), "r");
-    if (pipe == NULL) {
-        LOG(FATAL) << "Fail to popen `" << cmd << "'";
+    std::stringstream ss;
+    const int rc = butil::read_command_output(ss, cmd.c_str());
+    if (rc < 0) {
+        LOG(ERROR) << "Fail to popen `" << cmd << "'";
         return -1;
     }
-    char* line = NULL;
-    size_t line_len = 0;
-    ssize_t nr = 0;
-    while ((nr = getline(&line, &line_len, pipe)) != -1) {
-        butil::StringSplitter sp(line, ' ');
+    std::string line;
+    while (std::getline(ss, line)) {
+        butil::StringSplitter sp(line.c_str(), ' ');
         if (sp == NULL) {
             continue;
         }
@@ -381,8 +381,6 @@ static int ExtractSymbolsFromBinary(
     if (addr_map.find(lib_info.end_addr) == addr_map.end()) {
         addr_map[lib_info.end_addr] = std::string();
     }
-    pclose(pipe);
-    free(line);
     tm.stop();
     RPC_VLOG << "Loaded " << lib_info.path << " in " << tm.m_elapsed() << "ms";
     return 0;

src/brpc/builtin/threads_service.cpp

@@ -16,6 +16,7 @@
 
 #include "butil/time.h"
 #include "butil/logging.h"
+#include "butil/popen.h"
 #include "brpc/controller.h"           // Controller
 #include "brpc/closure_guard.h"        // ClosureGuard
 #include "brpc/builtin/threads_service.h"
@@ -37,21 +38,13 @@ void ThreadsService::default_method(::google::protobuf::RpcController* cntl_base
     std::string cmd = butil::string_printf("pstack %lld", (long long)getpid());
     butil::Timer tm;
     tm.start();
-    FILE* pipe = popen(cmd.c_str(), "r");
-    if (pipe == NULL) {
-        LOG(FATAL) << "Fail to popen `" << cmd << "'";
+    butil::IOBufBuilder pstack_output;
+    const int rc = butil::read_command_output(pstack_output, cmd.c_str());
+    if (rc < 0) {
+        LOG(ERROR) << "Fail to popen `" << cmd << "'";
         return;
     }
-    read_portal.append_from_file_descriptor(fileno(pipe), MAX_READ);
-    resp.swap(read_portal);
-    
-    // Call fread, otherwise following error will be reported:
-    //   sed: couldn't flush stdout: Broken pipe
-    // and pclose will fail:
-    //   CHECK failed: 0 == pclose(pipe): Resource temporarily unavailable
-    size_t fake_buf;
-    butil::ignore_result(fread(&fake_buf, sizeof(fake_buf), 1, pipe));
-    CHECK_EQ(0, pclose(pipe)) << berror();
+    pstack_output.move_to(resp);
     tm.stop();
     resp.append(butil::string_printf("\n\ntime=%lums", tm.m_elapsed()));
 }

src/brpc/server.h

@@ -376,7 +376,7 @@ public:
     struct MethodProperty {
         bool is_builtin_service;
         bool own_method_status;
-        // Parameters which have nothing to with management of services, but
+        // Parameters which have nothing to do with management of services, but
         // will be used when the service is queried.
         struct OpaqueParams {
             bool is_tabbed;

test/Makefile

@@ -112,6 +112,7 @@ TEST_BUTIL_SOURCES = \
     test_switches.cc \
     scoped_locale.cc \
     test_file_util_linux.cc \
+    popen_unittest.cpp \
     butil_unittest_main.cpp
 
 

test/popen_unittest.cpp

+// Copyright (c) 2017 Baidu.com, Inc. All Rights Reserved
+
+// Author: Zhangyi Chen (chenzhangyi01@baidu.com)
+// Date: 2017/11/06 10:57:08
+
+#include <gtest/gtest.h>
+#include <sstream>
+#include "butil/popen.h"
+#include "butil/errno.h"
+#include "butil/strings/string_piece.h"
+
+namespace {
+
+class PopenTest : public testing::Test {
+};
+
+TEST(PopenTest, sanity) {
+    std::ostringstream oss;
+    int rc = butil::read_command_output(oss, "echo \"Hello World\"");
+    ASSERT_EQ(0, rc) << berror(errno);
+    ASSERT_EQ("Hello World\n", oss.str());
+
+    oss.str("");
+    rc = butil::read_command_output(oss, "exit 1");
+    ASSERT_EQ(1, rc) << berror(errno);
+    ASSERT_TRUE(oss.str().empty()) << oss;
+    oss.str("");
+    rc = butil::read_command_output(oss, "kill -9 $$");
+    ASSERT_EQ(-1, rc);
+    ASSERT_EQ(errno, ECHILD);
+    ASSERT_TRUE(butil::StringPiece(oss.str()).ends_with("was killed by signal 9"));
+    oss.str("");
+    rc = butil::read_command_output(oss, "kill -15 $$");
+    ASSERT_EQ(-1, rc);
+    ASSERT_EQ(errno, ECHILD);
+    ASSERT_TRUE(butil::StringPiece(oss.str()).ends_with("was killed by signal 15"));
+
+    oss.str("");
+    ASSERT_EQ(0, butil::read_command_output(oss, "printf '=%.0s' {1..100000}"));
+    ASSERT_EQ(100000u, oss.str().length());
+    std::string expected;
+    expected.resize(100000, '=');
+    ASSERT_EQ(expected, oss.str());
+}
+
+}