Limit the number of simultaneous event logs.

webrtc/logging/rtc_event_log/rtc_event_log.cc

 /*
  *  Copyright (c) 2015 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 73 matching lines @@
   void LogAudioNetworkAdaptation(
       const AudioEncoderRuntimeConfig& config) override;
   void LogProbeClusterCreated(int id,
                               int bitrate_bps,
                               int min_probes,
                               int min_bytes) override;
   void LogProbeResultSuccess(int id, int bitrate_bps) override;
   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_;
 
   // Message queue for passing events to the logging thread.
   SwapQueue<std::unique_ptr<rtclog::Event> > event_queue_;
 
   RtcEventLogHelperThread helper_thread_;
   rtc::ThreadChecker thread_checker_;
 
   RTC_DISALLOW_COPY_AND_ASSIGN(RtcEventLogImpl);
@@ skipping 45 matching lines @@
   RTC_NOTREACHED();
   return rtclog::BweProbeResult::SUCCESS;
 }
 
 // The RTP and RTCP buffers reserve space for twice the expected number of
 // sent packets because they also contain received packets.
 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.
     : message_queue_(kControlMessagesPerSecond),
       event_queue_(kEventsPerSecond),
       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,
                                    int64_t max_size_bytes) {
   RTC_DCHECK(thread_checker_.CalledOnValidThread());
   RtcEventLogHelperThread::ControlMessage message;
   message.message_type = RtcEventLogHelperThread::ControlMessage::START_FILE;
   message.max_size_bytes = max_size_bytes <= 0
                                ? std::numeric_limits<int64_t>::max()
                                : max_size_bytes;
@@ skipping 367 matching lines @@
 
 void RtcEventLogImpl::StoreEvent(std::unique_ptr<rtclog::Event>* event) {
   RTC_DCHECK(event != nullptr);
   RTC_DCHECK(event->get() != nullptr);
   if (!event_queue_.Insert(event)) {
     LOG(LS_ERROR) << "WebRTC event log queue full. Dropping 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];
   int bytes_read = 0;
   std::unique_ptr<FileWrapper> dump_file(FileWrapper::Create());
   if (!dump_file->OpenFile(file_name.c_str(), true)) {
     return false;
   }
   ProtoString dump_buffer;
   while ((bytes_read = dump_file->Read(tmp_buffer, sizeof(tmp_buffer))) > 0) {
     dump_buffer.append(tmp_buffer, bytes_read);
   }
   dump_file->CloseFile();
   return result->ParseFromString(dump_buffer);
 }
 
 #endif  // ENABLE_RTC_EVENT_LOG
 
 // 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
 }
 
 std::unique_ptr<RtcEventLog> RtcEventLog::CreateNull() {
   return std::unique_ptr<RtcEventLog>(new RtcEventLogNullImpl());
 }
 
 }  // namespace webrtc