Allow multiple logging::LogMessage{Handler,Listener}s

base/logging.cc

Index: base/logging.cc
diff --git a/base/logging.cc b/base/logging.cc
index 0771b47c182e5c18c868f1124a0784bf411d74a9..1203975cceee9c399d4ffecfc3c8feafb72d24e9 100644
--- a/base/logging.cc
+++ b/base/logging.cc
@@ -49,15 +49,18 @@ typedef pthread_mutex_t* MutexHandle;
 #include <algorithm>
 #include <cstring>
 #include <ctime>
+#include <deque>
 #include <iomanip>
 #include <ostream>
 #include <string>
+#include <unordered_set>
 
 #include "base/base_switches.h"
 #include "base/command_line.h"
 #include "base/debug/alias.h"
 #include "base/debug/debugger.h"
 #include "base/debug/stack_trace.h"
+#include "base/lazy_instance.h"
 #include "base/posix/eintr_wrapper.h"
 #include "base/strings/string_piece.h"
 #include "base/strings/string_util.h"
@@ -123,8 +126,13 @@ bool show_error_dialogs = false;
 // An assert handler override specified by the client to be called instead of
 // the debug message dialog and process termination.
 LogAssertHandlerFunction log_assert_handler = nullptr;
-// A log message handler that gets notified of every log message we process.
-LogMessageHandlerFunction log_message_handler = nullptr;
+// Log message handlers that get notified of every log message we process.
+base::LazyInstance<std::deque<LogMessageHandler*>>::Leaky
+    log_message_handlers = LAZY_INSTANCE_INITIALIZER;
+// Log message listeners that get notified of every log message we process
+// before log message handlers.
+base::LazyInstance<std::unordered_set<LogMessageListener*>>::Leaky

[grt (UTC plus 2), 2016/07/27 20:38:20]

why an unordered set? are there really so many listeners that you need the extra
memory overhead for O(lgN) insert/remove? vector or deque seems adequate.

[wychen, 2016/08/01 16:12:25]

The number of listeners should be very low practically, so I'm not worried about
asymptotic complexity. BTW, I think insert/remove is O(1) here. The reason I
used unordered_set is to avoid subscribers to assume ordering.
LogMessageHandlers are run in stack order, because the handlers can sink the
message. The calling order of LogMessageListeners doesn't matter from the
perspective of LogMessage, but subscribers are free to add side effects inside
the listener functions, and we don't want these side effects to depend on
certain calling order. 

[grt (UTC plus 2), 2016/08/01 21:17:44]

Ouch. Apologies for the complexity goof. I haven't looked at the implementation
of unordered set, but my guess is that it uses a hash table. If so, you're
paying the memory cost of this for a small number of elements. I don't think
this is worthwhile. It's sufficient to document that subscribers must not make
any ordering assumptions; you don't need to impose a memory penalty to avoid the
appearance of ordering. If you think the properties of a set are important, use
a std::set; otherwise, use the most lightweight container possible. My guess is
that vector fits the bill.

[wychen, 2016/08/01 23:11:03]

Unordered set does use a hash table internally. Do you think it's a good idea to
use CallbackList underneath log_message_listeners while keeping the API the
same?

[grt (UTC plus 2), 2016/08/02 07:25:46]

I'm a bit divided. On the one hand, I prefer callbacks to single-method
interfaces. On the other hand, the subscription aspect of CallbackList can be a
little clunky to work with and you'd have to introduce Add/Remove methods. I'm
okay with keeping the interface as it is.

[wychen, 2016/08/04 03:53:55]

I meant keeping the single-method interface as is, but it uses CallbackList
internally. It is still scope-based from the users' perspective, and we don't
need to introduce add/remove methods. We add a callback in the constructor of
LogMessageListener, and the registration object would be in unique_ptr<> as a
member data. The unregistration would happen in destruction time automatically.

I think this is better than using locks like what I did.

+    log_message_listeners = LAZY_INSTANCE_INITIALIZER;
 
 // Helper functions to wrap platform differences.
 
@@ -342,6 +350,34 @@ void CloseLogFileUnlocked() {
 
 }  // namespace
 
+LogMessageHandler::LogMessageHandler() {
+  log_message_handlers.Get().push_front(this);
+}
+
+LogMessageHandler::~LogMessageHandler() {
+  auto& handlers = log_message_handlers.Get();
+  unsigned count = handlers.size();

[grt (UTC plus 2), 2016/07/27 20:38:20]

size_t

[wychen, 2016/08/01 16:12:25]

Done.

+  handlers.erase(std::remove(handlers.begin(), handlers.end(), this),
+      handlers.end());
+  DCHECK_EQ(count - 1, handlers.size());
+}
+
+unsigned LogMessageHandlerCountForTesting() {
+  return log_message_handlers.Get().size();
+}
+
+LogMessageListener::LogMessageListener() {
+  log_message_listeners.Get().insert(this);
+}
+
+LogMessageListener::~LogMessageListener() {
+  DCHECK_EQ(1u, log_message_listeners.Get().erase(this));
+}
+
+unsigned LogMessageListenerCountForTesting() {
+  return log_message_listeners.Get().size();
+}
+
 LoggingSettings::LoggingSettings()
     : logging_dest(LOG_DEFAULT),
       log_file(nullptr),
@@ -409,7 +445,9 @@ bool ShouldCreateLogMessage(int severity) {
   // Return true here unless we know ~LogMessage won't do anything. Note that
   // ~LogMessage writes to stderr if severity_ >= kAlwaysPrintErrorLevel, even
   // when g_logging_destination is LOG_NONE.
-  return g_logging_destination != LOG_NONE || log_message_handler ||
+  return g_logging_destination != LOG_NONE ||
+         !log_message_handlers.Get().empty() ||
+         !log_message_listeners.Get().empty() ||
          severity >= kAlwaysPrintErrorLevel;
 }
 
@@ -443,14 +481,6 @@ void SetLogAssertHandler(LogAssertHandlerFunction handler) {
   log_assert_handler = handler;
 }
 
-void SetLogMessageHandler(LogMessageHandlerFunction handler) {
-  log_message_handler = handler;
-}
-
-LogMessageHandlerFunction GetLogMessageHandler() {
-  return log_message_handler;
-}
-
 // Explicit instantiations for commonly used comparisons.
 template std::string* MakeCheckOpString<int, int>(
     const int&, const int&, const char* names);
@@ -537,12 +567,18 @@ LogMessage::~LogMessage() {
   stream_ << std::endl;
   std::string str_newline(stream_.str());
 
-  // Give any log message handler first dibs on the message.
-  if (log_message_handler &&
-      log_message_handler(severity_, file_, line_,
-                          message_start_, str_newline)) {
-    // The handler took care of it, no further processing.
-    return;
+  // Broadcast to log message listeners first.
+  for (auto* listeners : log_message_listeners.Get()) {

[grt (UTC plus 2), 2016/07/27 20:38:20]

listeners -> listener

[grt (UTC plus 2), 2016/07/28 06:28:47]

since logging may happen on any thread at any time, how are you going to
guarantee that one thread isn't adding or removing a listener/handler while
another is doing these iterations?

[wychen, 2016/08/01 16:12:25]

I've added Locks.

[wychen, 2016/08/01 16:12:25]

Done.

+    listeners->OnMessage(severity_, file_, line_, message_start_, str_newline);
+  }
+
+  // Give log message handlers first dibs on the message.
+  for (auto* handler : log_message_handlers.Get()) {
+    if (handler->OnMessage(severity_, file_, line_, message_start_,
+        str_newline)) {
+      // The handler took care of it, no further processing.
+      return;
+    }
   }
 
   if ((g_logging_destination & LOG_TO_SYSTEM_DEBUG_LOG) != 0) {