Fix error_log and log_errors

Summary: ini_set("error_log", "my_log_file.out") would redirect the entire server's logging to my_log_file.out. It also didn't look very thread safe. Reviewed By: @JoelMarcey Differential Revision: D2317672
facebook · Aug 6, 2015 · d43f26b · d43f26b
1 parent 024b0ea
commit d43f26b
Show file tree

Hide file tree

Showing 4 changed files with 31 additions and 27 deletions.
diff --git a/hphp/runtime/base/request-injection-data.cpp b/hphp/runtime/base/request-injection-data.cpp
@@ -440,13 +440,10 @@ void RequestInjectionData::threadInit() {
                        if (m_logErrors != on) {
                          if (on) {
                            if (!m_errorLog.empty()) {
-                             FILE *output = fopen(m_errorLog.data(), "a");
-                             if (output) {
-                               Logger::SetNewOutput(output);
-                             }
+                             Logger::SetThreadLog(m_errorLog.data(), true);
                            }
                          } else {
-                           Logger::SetNewOutput(nullptr);
+                           Logger::ClearThreadLog();
                          }
                        }
                        return true;
@@ -458,11 +455,8 @@ void RequestInjectionData::threadInit() {
                    "error_log",
                    IniSetting::SetAndGet<std::string>(
                      [this](const std::string& value) {
-                       if (m_logErrors && !m_errorLog.empty()) {
-                         FILE *output = fopen(m_errorLog.data(), "a");
-                         if (output) {
-                           Logger::SetNewOutput(output);
-                         }
+                       if (m_logErrors && !value.empty()) {
+                         Logger::SetThreadLog(value.data(), true);
                        }
                        return true;
                      },

diff --git a/hphp/runtime/server/source-root-info.cpp b/hphp/runtime/server/source-root-info.cpp
@@ -105,7 +105,7 @@ void SourceRootInfo::createFromCommonRoot(const String &sandboxName) {
   }
   String logPath = logsRoot + "/" + sandboxName + "_error.log";
   String accessLogPath = logsRoot + "/" + sandboxName + "_access.log";
-  if (!Logger::SetThreadLog(logPath.c_str())) {
+  if (!Logger::SetThreadLog(logPath.c_str(), false)) {
     Logger::Warning("Sandbox error log %s could not be opened",
                     logPath.c_str());
   }
@@ -176,7 +176,7 @@ void SourceRootInfo::createFromUserConfig() {
     if (lp.charAt(0) != '/') {
       lp = homePath + lp;
     }
-    if (!Logger::SetThreadLog(lp.c_str())) {
+    if (!Logger::SetThreadLog(lp.c_str(), false)) {
       Logger::Warning("Sandbox error log %s could not be opened",
                       lp.c_str());
     }

diff --git a/hphp/util/logger.cpp b/hphp/util/logger.cpp
@@ -166,11 +166,16 @@ void Logger::log(LogLevelType level, const std::string &msg,
   if (UseLogFile) {
     FILE *stdf = GetStandardOut(level);
     FILE *f;
-    if (UseCronolog) {
-      f = cronOutput.getOutputFile();
-      if (!f) f = stdf;
+    FILE *tf = threadData->log;
+    if (tf && threadData->threadLogOnly) {
+      f = tf;
     } else {
-      f = Output ? Output : stdf;
+      if (UseCronolog) {
+        f = cronOutput.getOutputFile();
+      } else {
+        f = Output;
+      }
+      if (!f) f = stdf;
     }
     std::string header, sheader;
     if (LogHeader) {
@@ -193,8 +198,7 @@ void Logger::log(LogLevelType level, const std::string &msg,
       bytes = fprintf(f, "%s%s%s", sheader.c_str(), escaped, ending);
     }
 
-    FILE *tf = threadData->log;
-    if (tf) {
+    if (tf && tf != f) {
       int threadBytes =
         fprintf(tf, "%s%s%s", header.c_str(), escaped, ending);
       fflush(tf);
@@ -269,15 +273,21 @@ char *Logger::EscapeString(const std::string &msg) {
   return new_str;
 }
 
-bool Logger::SetThreadLog(const char *file) {
-  return (s_threadData->log = fopen(file, "a")) != nullptr;
+bool Logger::SetThreadLog(const char *file, bool threadOnly) {
+  if (auto log = fopen(file, "a")) {
+    ClearThreadLog();
+    s_threadData->log = log;
+    s_threadData->threadLogOnly = threadOnly;
+    return true;
+  }
+  return false;
 }
 void Logger::ClearThreadLog() {
   ThreadData *threadData = s_threadData.get();
   if (threadData->log) {
     fclose(threadData->log);
+    threadData->log = nullptr;
   }
-  threadData->log = nullptr;
 }
 
 void Logger::SetThreadHook(PFUNC_LOG func, void *data) {

diff --git a/hphp/util/logger.h b/hphp/util/logger.h
@@ -79,7 +79,7 @@ class Logger {
   static void OnNewRequest();
   static void ResetRequestCount();
 
-  static bool SetThreadLog(const char *file);
+  static bool SetThreadLog(const char *file, bool threadOnly);
   static void ClearThreadLog();
   static void SetNewOutput(FILE *output);
   static void UnlimitThreadMessages();
@@ -106,12 +106,12 @@ class Logger {
 protected:
   class ThreadData {
   public:
-    ThreadData() : request(0), message(0), log(nullptr), hook(nullptr) {}
-    int request;
-    int message;
+    int request{0};
+    int message{0};
     LogFileFlusher flusher;
-    FILE *log;
-    PFUNC_LOG hook;
+    FILE *log{nullptr};
+    bool threadLogOnly{false};
+    PFUNC_LOG hook{nullptr};
     void *hookData;
   };
   static DECLARE_THREAD_LOCAL(ThreadData, s_threadData);