Limit the number of simultaneous event logs.

webrtc/logging/rtc_event_log/rtc_event_log.cc

Index: webrtc/logging/rtc_event_log/rtc_event_log.cc
diff --git a/webrtc/logging/rtc_event_log/rtc_event_log.cc b/webrtc/logging/rtc_event_log/rtc_event_log.cc
index 0c2b15d13c0407c78a25a2fc3ac8a5e409025a64..95c085d39b3df518c4dc79a956244101d454d7c4 100644
--- a/webrtc/logging/rtc_event_log/rtc_event_log.cc
+++ b/webrtc/logging/rtc_event_log/rtc_event_log.cc
@@ -91,12 +91,17 @@ class RtcEventLogImpl final : public RtcEventLog {
   void LogProbeResultFailure(int id,
                              ProbeFailureReason failure_reason) override;
 
+  static bool AllowNewEventLog();
+
  private:
   void StoreEvent(std::unique_ptr<rtclog::Event>* event);
   void LogProbeResult(int id,
                       rtclog::BweProbeResult::ResultType result,
                       int bitrate_bps);
 
+  static rtc::CriticalSection crit_;

[tommi, 2017/06/27 17:35:19]

this is a complex global == cl will get reverted right away

+  static int log_count GUARDED_BY(crit_);
+
   // Message queue for passing control messages to the logging thread.
   SwapQueue<RtcEventLogHelperThread::ControlMessage> message_queue_;
 
@@ -162,6 +167,9 @@ static const int kEventsPerSecond = 1000;
 static const int kControlMessagesPerSecond = 10;
 }  // namespace
 
+rtc::CriticalSection RtcEventLogImpl::crit_;
+int RtcEventLogImpl::log_count = 0;

[tommi, 2017/06/27 17:35:19]

log_count is missing the postfix _
actually... since it's global, perhaps it should have a g_ prefix?

Regardless, I think we need to change the approach to not use a lock but only
use a single atomic variable (assuming it really is accessed from multiple
threads).

+
 // RtcEventLogImpl member functions.
 RtcEventLogImpl::RtcEventLogImpl()
     // Allocate buffers for roughly one second of history.
@@ -170,11 +178,17 @@ RtcEventLogImpl::RtcEventLogImpl()
       helper_thread_(&message_queue_, &event_queue_),
       thread_checker_() {
   thread_checker_.DetachFromThread();
+  rtc::CritScope lock(&crit_);
+  log_count++;
+  RTC_DCHECK_GE(log_count, 1);
 }
 
 RtcEventLogImpl::~RtcEventLogImpl() {
   // The RtcEventLogHelperThread destructor closes the file
   // and waits for the thread to terminate.
+  rtc::CritScope lock(&crit_);
+  log_count--;
+  RTC_DCHECK_GE(log_count, 0);
 }
 
 bool RtcEventLogImpl::StartLogging(const std::string& file_name,
@@ -562,6 +576,16 @@ void RtcEventLogImpl::StoreEvent(std::unique_ptr<rtclog::Event>* event) {
   helper_thread_.SignalNewEvent();
 }
 
+bool RtcEventLogImpl::AllowNewEventLog() {
+  constexpr int kMaxLogCount = 5;
+  rtc::CritScope lock(&crit_);
+  if (log_count < kMaxLogCount)
+    return true;
+  LOG(LS_WARNING) << "Denied creation of additional WebRTC event logs. "

[tommi, 2017/06/27 17:35:19]

there isn't a need to make this call while holding a lock btw.  Logging can
invoke quite a bit of code, including grabbing additional, potentially
contended, locks.

+                  << log_count << " logs open already.";
+  return false;
+}
+
 bool RtcEventLog::ParseRtcEventLog(const std::string& file_name,
                                    rtclog::EventStream* result) {
   char tmp_buffer[1024];
@@ -583,7 +607,17 @@ bool RtcEventLog::ParseRtcEventLog(const std::string& file_name,
 // RtcEventLog member functions.
 std::unique_ptr<RtcEventLog> RtcEventLog::Create() {
 #ifdef ENABLE_RTC_EVENT_LOG
-  return std::unique_ptr<RtcEventLog>(new RtcEventLogImpl());
+  if (RtcEventLogImpl::AllowNewEventLog()) {

[tommi, 2017/06/27 17:35:19]

instead of this approach, what about something like:

auto ret = AtomicIncrement(&g_event_log_count);
if (ret > kMax) {
  AtomicDecrement(&g_event_log_count);
  return std::unique_ptr<RtcEventLog>(new RtcEventLogNullImpl());
}

return std::unique_ptr<RtcEventLog>(new RtcEventLogImpl());

+    // Note that there is a gap between checking the log count and creating a
+    // new log. Hence it is theoretically possible to create more than
+    // kMaxLogCount logs if multiple thread check the log count simultaneously,
+    // This is unlikely to happen in practice, and not harmful if it does since
+    // we don't care about the exact value of kMaxLogCount. The purpose is just
+    // to avoid creating too many logging threads.
+    return std::unique_ptr<RtcEventLog>(new RtcEventLogImpl());
+  } else {

[tommi, 2017/06/27 17:35:19]

no need for 'else' since the previous conditional always returns

+    return std::unique_ptr<RtcEventLog>(new RtcEventLogNullImpl());
+  }
 #else
   return std::unique_ptr<RtcEventLog>(new RtcEventLogNullImpl());
 #endif  // ENABLE_RTC_EVENT_LOG