trafficserver-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From jpe...@apache.org
Subject git commit: TS-2757: logging crashes on reconfiguration
Date Thu, 01 May 2014 04:10:00 GMT
Repository: trafficserver
Updated Branches:
  refs/heads/master 4bd4b0159 -> 929296988


TS-2757: logging crashes on reconfiguration

The way that LogObjectManager transfers LogObjects to the new manager
when logging is reconfigured is prone to crashes since the previous
object manager can be used after the objects have been transferred
out of it. The solution to this is to persist the objects in the
manager for the whole of the manager's lifetime. This require adding
reference counting so that multiple managers can safely share
ownership of the same objects.

Added a TSQA test to exercise this code path. Plugged a LogFormat
leak when creating new TextLogObjects.


Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo
Commit: http://git-wip-us.apache.org/repos/asf/trafficserver/commit/92929698
Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/92929698
Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/92929698

Branch: refs/heads/master
Commit: 929296988bbb13561af8420153666e190cd2a259
Parents: 4bd4b01
Author: James Peach <jpeach@apache.org>
Authored: Tue Apr 29 14:54:27 2014 -0700
Committer: James Peach <jpeach@apache.org>
Committed: Wed Apr 30 21:08:54 2014 -0700

----------------------------------------------------------------------
 CHANGES                      |   2 +
 ci/tsqa/functions            |   6 +
 ci/tsqa/test-log-refcounting | 112 +++++++++++++++
 lib/records/RecHttp.cc       |   2 +-
 lib/ts/Vec.h                 |   2 +-
 proxy/InkAPI.cc              |  24 ++--
 proxy/logging/LogConfig.cc   |   5 +
 proxy/logging/LogObject.cc   | 280 ++++++++++++++++----------------------
 proxy/logging/LogObject.h    |  41 +++---
 9 files changed, 273 insertions(+), 201 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/trafficserver/blob/92929698/CHANGES
----------------------------------------------------------------------
diff --git a/CHANGES b/CHANGES
index 84d86c6..ba8e3f6 100644
--- a/CHANGES
+++ b/CHANGES
@@ -1,6 +1,8 @@
                                                          -*- coding: utf-8 -*-
 Changes with Apache Traffic Server 5.0.0
 
+  *) [TS-2757] Fix logging crashes on reconfiguration.
+
   *) [TS-2770] Allow proxy.config.log.rolling_interval_sec to be as low as 60sec.
 
   *) [TS-2763] Remove the unused proxy.config.log.xuid_logging_enabled config setting.

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/92929698/ci/tsqa/functions
----------------------------------------------------------------------
diff --git a/ci/tsqa/functions b/ci/tsqa/functions
index 472b453..46e7df1 100644
--- a/ci/tsqa/functions
+++ b/ci/tsqa/functions
@@ -175,6 +175,12 @@ startup() {
 
 # Shut down Traffic Server.
 shutdown() {
+
+  # Quick'n'dirty cleanup of background jobs.
+  jobs -p | while read pid ; do
+    kill $pid
+  done
+
   local pid=$(pidof cop)
   if [[ -z "$pid" ]] ; then
     return

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/92929698/ci/tsqa/test-log-refcounting
----------------------------------------------------------------------
diff --git a/ci/tsqa/test-log-refcounting b/ci/tsqa/test-log-refcounting
new file mode 100755
index 0000000..3340a7c
--- /dev/null
+++ b/ci/tsqa/test-log-refcounting
@@ -0,0 +1,112 @@
+#! /usr/bin/env bash
+
+#  Licensed to the Apache Software Foundation (ASF) under one
+#  or more contributor license agreements.  See the NOTICE file
+#  distributed with this work for additional information
+#  regarding copyright ownership.  The ASF licenses this file
+#  to you under the Apache License, Version 2.0 (the
+#  "License"); you may not use this file except in compliance
+#  with the License.  You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+#  Unless required by applicable law or agreed to in writing, software
+#  distributed under the License is distributed on an "AS IS" BASIS,
+#  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+#  See the License for the specific language governing permissions and
+#  limitations under the License.
+
+TSQA_TSXS=${TSQA_TSXS:-/opt/ats/bin/tsxs}
+TSQA_TESTNAME=$(basename $0)
+source $(dirname $0)/functions
+
+COUNT=${COUNT:-100}
+SERVER_PORT=${SERVER_PORT:-4000}
+RELOAD_INTERVAL_SECS=${RELOAD_INTERVAL_SECS:-30}
+
+bootstrap
+
+# Force a logging subsystem reload.
+reload() {
+  while sleep $RELOAD_INTERVAL_SECS ; do
+    msg reloading logging configuration
+    tsexec traffic_line -s proxy.config.log.hostname -v "$(date)"
+    tsexec traffic_line -x
+  done
+}
+
+if [ ! -x $(bindir)/jtest ] ; then
+  fatal "missing jtest program; rebuild with --enable-test-tools"
+fi
+
+cat >$TSQA_ROOT/$(sysconfdir)/remap.config <<REMAP
+map http://jtest.trafficserver.apache.org:$SERVER_PORT http://127.0.0.1:$SERVER_PORT
+REMAP
+
+# Configure the tcpinfo plugin so that we have some API log objects.
+cat >$TSQA_ROOT/$(sysconfdir)/plugin.config <<REMAP
+tcpinfo.so --log-file=tcpinfo1 --hooks=ssn_start,txn_start,send_resp_hdr,ssn_close,txn_close
--log-level=2
+tcpinfo.so --log-file=tcpinfo2 --hooks=ssn_start,txn_start,send_resp_hdr,ssn_close,txn_close
--log-level=2
+tcpinfo.so --log-file=tcpinfo3 --hooks=ssn_start,txn_start,send_resp_hdr,ssn_close,txn_close
--log-level=2
+tcpinfo.so --log-file=tcpinfo4 --hooks=ssn_start,txn_start,send_resp_hdr,ssn_close,txn_close
--log-level=2
+REMAP
+
+# If Traffic Server is not up, bring it up ...
+alive cop || startup || fatal unable to start Traffic Server
+trap shutdown 0 EXIT
+
+# Wait for traffic_manager to start.
+alive manager
+alive server
+msgwait 2
+
+# Logging configuration ...
+tsexec traffic_line -s proxy.config.log.max_space_mb_for_logs -v 10
+tsexec traffic_line -s proxy.config.log.max_space_mb_for_orphan_logs -v 10
+tsexec traffic_line -s proxy.config.log.squid_log_enabled -v 1
+tsexec traffic_line -s proxy.config.log.squid_log_is_ascii -v 1
+# Roll every megabyte ...
+tsexec traffic_line -s proxy.config.log.rolling_interval_sec -v 60
+tsexec traffic_line -s proxy.config.log.rolling_size_mb -v 1
+# Don't declare log space exhausted until there is < 1MB free.
+tsexec traffic_line -s proxy.config.log.max_space_mb_headroom -v 1
+# Flush log buffers every second.
+tsexec traffic_line -s proxy.config.log.max_secs_per_buffer -v 1
+
+# Enable logging diagnostics for test debugging. If you enable this
+# for the test itself, the diagnostics log will exhaust all the logging
+# space and everything will go to hell.
+#tsexec traffic_line -s proxy.config.diags.debug.tags -v log
+
+# The sleep is needed to let Traffic Server schedule the config change.
+msgwait 4 to restart with updated logging configuration
+# XXX: this needs a full bounce
+tsexec traffic_line -L
+
+# Wait for traffic_manager to start.
+alive manager
+alive server
+msgwait 10
+
+msg starting config reload every $RELOAD_INTERVAL_SECS seconds
+reload&
+
+for i in $(seq $COUNT) ; do
+  msg $(bindir)/jtest --proxy_port $PORT --proxy_host 127.0.0.1 \
+    --server_port $SERVER_PORT --server_host jtest.trafficserver.apache.org \
+    --clients 10 --test_time 3600
+  $(bindir)/jtest --proxy_port $PORT --proxy_host 127.0.0.1 \
+    --server_port $SERVER_PORT --server_host jtest.trafficserver.apache.org \
+    --clients 10 --test_time 60
+
+  if [ -r $(logdir)/squid.log ]; then
+    fatal squid.log is missing
+  fi
+
+  # Check for a crash ...
+  crash
+done
+
+exit $TSQA_FAIL
+
+# vim: set sw=2 ts=2 et :

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/92929698/lib/records/RecHttp.cc
----------------------------------------------------------------------
diff --git a/lib/records/RecHttp.cc b/lib/records/RecHttp.cc
index fc75907..83e8d93 100644
--- a/lib/records/RecHttp.cc
+++ b/lib/records/RecHttp.cc
@@ -157,7 +157,7 @@ HttpProxyPort::loadDefaultIfEmpty(Group& ports) {
 
 bool
 HttpProxyPort::loadValue(Vec<self>& ports, char const* text) {
-  int n_elts = ports.length(); // remember this.
+  unsigned n_elts = ports.length(); // remember this.
   if (text && *text) {
     Tokenizer tokens(", ");
     int n_ports = tokens.Initialize(text);

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/92929698/lib/ts/Vec.h
----------------------------------------------------------------------
diff --git a/lib/ts/Vec.h b/lib/ts/Vec.h
index 63a13ec..3deef08 100644
--- a/lib/ts/Vec.h
+++ b/lib/ts/Vec.h
@@ -106,7 +106,7 @@ class Vec {
   C &first() const { return v[0]; }
   C &last() const { return v[n-1]; }
   Vec<C,A,S>& operator=(Vec<C,A,S> &v) { this->copy(v); return *this;
}
-  int length () const { return n; }
+  unsigned length () const { return n; }
   // vector::size() intentionally not implemented because it should mean "bytes" not count
of elements
   int write(int fd);
   int read(int fd);

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/92929698/proxy/InkAPI.cc
----------------------------------------------------------------------
diff --git a/proxy/InkAPI.cc b/proxy/InkAPI.cc
index 38e143c..ecaa2d2 100644
--- a/proxy/InkAPI.cc
+++ b/proxy/InkAPI.cc
@@ -6801,7 +6801,7 @@ TSTextLogObjectCreate(const char *filename, int mode, TSTextLogObject
*new_objec
   sdk_assert(sdk_sanity_check_null_ptr((void*)filename) == TS_SUCCESS);
   sdk_assert(sdk_sanity_check_null_ptr((void*)new_object) == TS_SUCCESS);
 
-  if (mode<0 || mode>= TS_LOG_MODE_INVALID_FLAG) {
+  if (mode <0 || mode >= TS_LOG_MODE_INVALID_FLAG) {
     *new_object = NULL;
     return TS_ERROR;
   }
@@ -6814,21 +6814,21 @@ TSTextLogObjectCreate(const char *filename, int mode, TSTextLogObject
*new_objec
                                               Log::config->rolling_interval_sec,
                                               Log::config->rolling_offset_hr,
                                               Log::config->rolling_size_mb));
-  if (tlog) {
-    int err = (mode & TS_LOG_MODE_DO_NOT_RENAME ?
-               Log::config->log_object_manager.manage_api_object(tlog, 0) :
-               Log::config->log_object_manager.manage_api_object(tlog));
-    if (err != LogObjectManager::NO_FILENAME_CONFLICTS) {
-      delete tlog;
-      *new_object = NULL;
-      return TS_ERROR;
-    }
-  } else {
+  if (tlog == NULL) {
     *new_object = NULL;
     return TS_ERROR;
   }
-  *new_object = (TSTextLogObject) tlog;
 
+  int err = (mode & TS_LOG_MODE_DO_NOT_RENAME ?
+             Log::config->log_object_manager.manage_api_object(tlog, 0) :
+             Log::config->log_object_manager.manage_api_object(tlog));
+  if (err != LogObjectManager::NO_FILENAME_CONFLICTS) {
+    delete tlog;
+    *new_object = NULL;
+    return TS_ERROR;
+  }
+
+  *new_object = (TSTextLogObject) tlog;
   return TS_SUCCESS;
 }
 

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/92929698/proxy/logging/LogConfig.cc
----------------------------------------------------------------------
diff --git a/proxy/logging/LogConfig.cc b/proxy/logging/LogConfig.cc
index feb1755..40ea3e8 100644
--- a/proxy/logging/LogConfig.cc
+++ b/proxy/logging/LogConfig.cc
@@ -1310,6 +1310,11 @@ bool LogConfig::space_to_write(int64_t bytes_to_write)
 
   space = ((logical_space_used<config_space) && (physical_space_left> partition_headroom));
 
+  Debug("logspace", "logical space used %" PRId64 ", configured space %" PRId64
+      ", physical space left %" PRId64 ", partition headroom %" PRId64 ", space %s available",
+      logical_space_used, config_space, physical_space_left, partition_headroom,
+      space ? "is" : "is not");
+
   return space;
 }
 

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/92929698/proxy/logging/LogObject.cc
----------------------------------------------------------------------
diff --git a/proxy/logging/LogObject.cc b/proxy/logging/LogObject.cc
index c67b1a0..52ace03 100644
--- a/proxy/logging/LogObject.cc
+++ b/proxy/logging/LogObject.cc
@@ -100,7 +100,6 @@ LogObject::LogObject(const LogFormat *format, const char *log_dir,
       m_rolling_offset_hr (rolling_offset_hr),
       m_rolling_size_mb (rolling_size_mb),
       m_last_roll_time(0),
-      m_ref_count (0),
       m_buffer_manager_idx(0)
 {
     ink_assert (format != NULL);
@@ -144,8 +143,7 @@ LogObject::LogObject(LogObject& rhs)
     m_signature(rhs.m_signature),
     m_flush_threads(rhs.m_flush_threads),
     m_rolling_interval_sec(rhs.m_rolling_interval_sec),
-    m_last_roll_time(rhs.m_last_roll_time),
-    m_ref_count(0)
+    m_last_roll_time(rhs.m_last_roll_time)
 {
     m_format = new LogFormat(*(rhs.m_format));
     m_buffer_manager = new LogBufferManager[m_flush_threads];
@@ -182,10 +180,6 @@ LogObject::~LogObject()
 {
   Debug("log-config", "entering LogObject destructor, this=%p", this);
 
-  while (m_ref_count > 0) {
-    Debug("log-config", "LogObject refcount = %d, waiting for zero", m_ref_count);
-  }
-
   preproc_buffers();
 
   // here we need to free LogHost if it is remote logging.
@@ -567,8 +561,6 @@ LogObject::log(LogAccess * lad, const char *text_entry)
     return Log::FAIL;
   }
 
-  RefCounter counter(&m_ref_count);     // scope exit will decrement
-
   if (lad && m_filter_list.toss_this_entry(lad)) {
     Debug("log", "entry filtered, skipping ...");
     return Log::SKIP;
@@ -717,13 +709,13 @@ LogObject::_setup_rolling(Log::RollingEnabledValues rolling_enabled,
int rolling
 }
 
 
-int
+unsigned
 LogObject::roll_files(long time_now)
 {
   if (!m_rolling_enabled)
     return 0;
 
-  int num_rolled = 0;
+  unsigned num_rolled = 0;
   bool roll_on_time = false;
   bool roll_on_size = false;
 
@@ -779,10 +771,10 @@ LogObject::roll_files(long time_now)
 }
 
 
-int
+unsigned
 LogObject::_roll_files(long last_roll_time, long time_now)
 {
-  int num_rolled = 0;
+  unsigned num_rolled = 0;
 
   if (m_logFile) {
     // no need to roll if object writes to a pipe
@@ -829,12 +821,14 @@ LogObject::do_filesystem_checks()
 /*-------------------------------------------------------------------------
   TextLogObject::TextLogObject
   -------------------------------------------------------------------------*/
+const LogFormat * TextLogObject::textfmt = MakeTextLogFormat();
+
 TextLogObject::TextLogObject(const char *name, const char *log_dir,
                              bool timestamps, const char *header,
                              Log::RollingEnabledValues rolling_enabled, int flush_threads,
                              int rolling_interval_sec, int rolling_offset_hr,
                              int rolling_size_mb)
-  : LogObject(MakeTextLogFormat(), log_dir, name, LOG_FILE_ASCII, header,
+  : LogObject(TextLogObject::textfmt, log_dir, name, LOG_FILE_ASCII, header,
               rolling_enabled, flush_threads, rolling_interval_sec,
               rolling_offset_hr, rolling_size_mb)
 {
@@ -889,66 +883,27 @@ TextLogObject::va_write(const char *format, va_list ap)
   -------------------------------------------------------------------------*/
 
 LogObjectManager::LogObjectManager()
-   : _numObjects(0), _maxObjects(LOG_OBJECT_ARRAY_DELTA), _numAPIobjects(0), _maxAPIobjects(LOG_OBJECT_ARRAY_DELTA)
 {
-  _objects = new LogObject *[_maxObjects];
-  _APIobjects = new LogObject *[_maxAPIobjects];
   _APImutex = NEW(new ink_mutex);
   ink_mutex_init(_APImutex, "_APImutex");
-
-  memset(_objects, 0, sizeof(LogObject *) * _maxObjects);
-  memset(_APIobjects, 0, sizeof(LogObject *) * _maxAPIobjects);
 }
 
 LogObjectManager::~LogObjectManager()
 {
-  for (unsigned i = 0; i < _maxObjects; i++) {
-    delete _objects[i];
+  for (unsigned i = 0; i < _objects.length(); ++i) {
+    if (REF_COUNT_OBJ_REFCOUNT_DEC(_objects[i]) == 0) {
+      delete _objects[i];
+    }
   }
 
-  for (unsigned i = 0; i < _maxAPIobjects; i++) {
-    delete _APIobjects[i];
+  for (unsigned i = 0; i < _APIobjects.length(); ++i) {
+    if (REF_COUNT_OBJ_REFCOUNT_DEC(_APIobjects[i]) == 0) {
+      delete _APIobjects[i];
+    }
   }
 
-  delete[] _objects;
-  delete[] _APIobjects;
   delete _APImutex;
 }
-void
-LogObjectManager::_add_object(LogObject * object)
-{
-  if (_numObjects == _maxObjects) {
-    _maxObjects += LOG_OBJECT_ARRAY_DELTA;
-    LogObject **_new_objects = new LogObject *[_maxObjects];
-
-    memset(_new_objects, 0, sizeof(LogObject *) * _maxObjects);
-    memcpy(_new_objects, _objects, sizeof(LogObject *) * _numObjects);
-    delete[]_objects;
-    _objects = _new_objects;
-  }
-
-  _objects[_numObjects++] = object;
-}
-
-
-// _add_api_object must be called with the _APImutex held
-//
-void
-LogObjectManager::_add_api_object(LogObject * object)
-{
-  if (_numAPIobjects == _maxAPIobjects) {
-    _maxAPIobjects += LOG_OBJECT_ARRAY_DELTA;
-    LogObject **_new_objects = new LogObject *[_maxAPIobjects];
-
-    memset(_new_objects, 0, sizeof(LogObject *) * _maxAPIobjects);
-    memcpy(_new_objects, _APIobjects, sizeof(LogObject *) * _numAPIobjects);
-    delete[]_APIobjects;
-    _APIobjects = _new_objects;
-  }
-
-  _APIobjects[_numAPIobjects++] = object;
-}
-
 
 int
 LogObjectManager::_manage_object(LogObject * log_object, bool is_api_object, int maxConflicts)
@@ -978,12 +933,15 @@ LogObjectManager::_manage_object(LogObject * log_object, bool is_api_object,
int
 
         // no conflicts, add object to the list of managed objects
         //
+        REF_COUNT_OBJ_REFCOUNT_INC(log_object);
         if (is_api_object) {
-          _add_api_object(log_object);
+          _APIobjects.push_back(log_object);
         } else {
-          _add_object(log_object);
+          _objects.push_back(log_object);
         }
 
+        ink_release_assert(retVal == NO_FILENAME_CONFLICTS);
+
         Debug("log", "LogObjectManager managing object %s (%s) "
               "[signature = %" PRIu64 ", address = %p]",
               log_object->get_base_filename(),
@@ -1114,17 +1072,16 @@ LogObjectManager::_solve_filename_conflicts(LogObject * log_object,
int maxConfl
 
 
 bool
-LogObjectManager::_has_internal_filename_conflict(const char *filename, LogObject ** objects,
int numObjects)
+LogObjectManager::_has_internal_filename_conflict(const char *filename, LogObjectList&
objects)
 {
-  for (int i = 0; i < numObjects; i++) {
-    LogObject *obj = objects[i];
+  for (unsigned i = 0; i < objects.length(); i++) {
 
-    if (!obj->is_collation_client()) {
+    if (!objects[i]->is_collation_client()) {
       // an internal conflict exists if two objects request the
       // same filename, regardless of the object signatures, since
       // two objects writing to the same file would produce a
       // log with duplicate entries and non monotonic timestamps
-      if (strcmp(obj->get_full_filename(), filename) == 0) {
+      if (strcmp(objects[i]->get_full_filename(), filename) == 0) {
         return true;
       }
     }
@@ -1139,8 +1096,8 @@ LogObjectManager::_solve_internal_filename_conflicts(LogObject *log_object,
int
   int retVal = NO_FILENAME_CONFLICTS;
   const char *filename = log_object->get_full_filename();
 
-  if (_has_internal_filename_conflict(filename, _objects, _numObjects) ||
-      _has_internal_filename_conflict(filename, _APIobjects, _numAPIobjects)) {
+  if (_has_internal_filename_conflict(filename, _objects) ||
+      _has_internal_filename_conflict(filename, _APIobjects)) {
     if (fileNum < maxConflicts) {
       char new_name[MAXPATHLEN];
 
@@ -1163,32 +1120,31 @@ LogObjectManager::_solve_internal_filename_conflicts(LogObject *log_object,
int
 LogObject *
 LogObjectManager::get_object_with_signature(uint64_t signature)
 {
-  for (size_t i = 0; i < _numObjects; i++) {
-    LogObject *obj = _objects[i];
+  for (unsigned i = 0; i < this->_objects.length(); i++) {
+    LogObject * obj = this->_objects[i];
 
     if (obj->get_signature() == signature) {
       return obj;
     }
   }
-  return (LogObject *) (0);
+  return NULL;
 }
 
 
 void
 LogObjectManager::check_buffer_expiration(long time_now)
 {
-  size_t i;
-
-  for (i = 0; i < _numObjects; i++) {
-    if (_objects[i]) {
-      _objects[i]->check_buffer_expiration(time_now);
-    }
+  for (unsigned i = 0; i < this->_objects.length(); i++) {
+    this->_objects[i]->check_buffer_expiration(time_now);
   }
-  for (i = 0; i < _numAPIobjects; i++) {
-    if (_APIobjects[i]) {
-      _APIobjects[i]->check_buffer_expiration(time_now);
-    }
+
+  ACQUIRE_API_MUTEX("A LogObjectManager::check_buffer_expiration");
+
+  for (unsigned i = 0; i < this->_APIobjects.length(); i++) {
+    this->_APIobjects[i]->check_buffer_expiration(time_now);
   }
+
+  RELEASE_API_MUTEX("R LogObjectManager::check_buffer_expiration");
 }
 
 size_t
@@ -1196,18 +1152,18 @@ LogObjectManager::preproc_buffers(int idx)
 {
   size_t buffers_preproced = 0;
 
-  for (unsigned i = 0; i < _numObjects; i++) {
-    if (_objects[i]) {
-      buffers_preproced += _objects[i]->preproc_buffers(idx);
-    }
+  for (unsigned i = 0; i < this->_objects.length(); i++) {
+    buffers_preproced += this->_objects[i]->preproc_buffers(idx);
   }
 
-  for (unsigned i = 0; i < _numAPIobjects; i++) {
-    if (_APIobjects[i]) {
-      buffers_preproced += _APIobjects[i]->preproc_buffers(idx);
-    }
+  ACQUIRE_API_MUTEX("A LogObjectManager::preproc_buffers");
+
+  for (unsigned i = 0; i < this->_APIobjects.length(); i++) {
+    buffers_preproced += this->_APIobjects[i]->preproc_buffers(idx);
   }
 
+  RELEASE_API_MUTEX("R LogObjectManager::preproc_buffers");
+
   return buffers_preproced;
 }
 
@@ -1217,22 +1173,17 @@ LogObjectManager::unmanage_api_object(LogObject * logObject)
 {
   ACQUIRE_API_MUTEX("A LogObjectManager::unmanage_api_object");
 
-  for (size_t i = 0; i < _numAPIobjects; i++) {
-    if (logObject == _APIobjects[i]) {
-
-      // Force a buffer flush, then schedule this LogObject to be deleted on the eventProcessor.
-      logObject->force_new_buffer();
-      new_Deleter(logObject, HRTIME_SECONDS(60));
+  if (this->_APIobjects.in(logObject)) {
+    this->_APIobjects.remove(logObject);
 
-      for (size_t j = i + 1; j < _numAPIobjects; j++) {
-        _APIobjects[j - 1] = _APIobjects[j];
-      }
+    // Force a buffer flush, then schedule this LogObject to be deleted on the eventProcessor.
+    logObject->force_new_buffer();
+    new_Derefer(logObject, HRTIME_SECONDS(60));
 
-      --_numAPIobjects;
-      RELEASE_API_MUTEX("R LogObjectManager::unmanage_api_object");
-      return true;
-    }
+    RELEASE_API_MUTEX("R LogObjectManager::unmanage_api_object");
+    return true;
   }
+
   RELEASE_API_MUTEX("R LogObjectManager::unmanage_api_object");
   return false;
 }
@@ -1241,7 +1192,7 @@ LogObjectManager::unmanage_api_object(LogObject * logObject)
 void
 LogObjectManager::add_filter_to_all(LogFilter * filter)
 {
-  for (size_t i = 0; i < _numObjects; i++) {
+  for (unsigned i = 0; i < this->_objects.length(); i++) {
     _objects[i]->add_filter(filter);
   }
 }
@@ -1253,7 +1204,7 @@ LogObjectManager::open_local_pipes()
   // for all local objects that write to a pipe, call open_file to force
   // the creation of the pipe so that any potential reader can see it
   //
-  for (size_t i = 0; i < _numObjects; i++) {
+  for (unsigned i = 0; i < this->_objects.length(); i++) {
     LogObject *obj = _objects[i];
     if (obj->writes_to_pipe() && !obj->is_collation_client()) {
       obj->m_logFile->open_file();
@@ -1267,111 +1218,113 @@ LogObjectManager::transfer_objects(LogObjectManager & old_mgr)
 {
   unsigned num_kept_objects = 0;
 
+  Debug("log-config-transfer", "transferring objects from LogObjectManager %p, to %p", &old_mgr,
this);
+
   if (is_debug_tag_set("log-config-transfer")) {
     Debug("log-config-transfer", "TRANSFER OBJECTS: list of old objects");
-    for (unsigned i = 0; i < old_mgr._numObjects; i++) {
+    for (unsigned i = 0; i < old_mgr._objects.length(); i++) {
       Debug("log-config-transfer", "%s", old_mgr._objects[i]->get_original_filename());
     }
 
     Debug("log-config-transfer", "TRANSFER OBJECTS : list of new objects");
-    for (unsigned i = 0; i < _numObjects; i++) {
+    for (unsigned i = 0; i < this->_objects.length(); i++) {
       Debug("log-config-transfer", "%s", _objects[i]->get_original_filename());
     }
   }
 
-  // Transfer the API objects to the new manager.
-  for (unsigned i = 0; i < old_mgr._numAPIobjects; i++) {
-    _add_api_object(old_mgr._APIobjects[i]);
+  // Transfer the API objects from the old manager. The old manager will retain its refcount.
+  for (unsigned i = 0; i < old_mgr._APIobjects.length(); ++i) {
+    manage_api_object(old_mgr._APIobjects[i]);
   }
 
-  // And nuke them from the old manager ...
-  memset(old_mgr._APIobjects, 0, sizeof(LogObject *) * old_mgr._numAPIobjects);
-  old_mgr._numAPIobjects = 0;
-
-  for (unsigned i = 0; i < old_mgr._numObjects; ++i) {
+  for (unsigned i = 0; i < old_mgr._objects.length(); ++i) {
     LogObject * old_obj = old_mgr._objects[i];
     LogObject * new_obj;
 
     Debug("log-config-transfer", "examining existing object %s", old_obj->get_base_filename());
 
     // See if any of the new objects is just a copy of an old one. If so, transfer the
-    // old one to the new manager and delete the new one.
-    if (num_kept_objects < _numObjects) {
-      for (unsigned j = 0; j < _numObjects; j++) {
-        new_obj = _objects[j];
+    // old one to the new manager and delete the new one. We don't use Vec::in here because
+    // we need to compare the object hash, not the pointers.
+    for (unsigned j = 0; j < _objects.length(); j++) {
+      new_obj = _objects[j];
 
-        Debug("log-config-transfer",
-              "comparing existing object %s to new object %s", old_obj->get_base_filename(),
new_obj->get_base_filename());
+      Debug("log-config-transfer",
+            "comparing existing object %s to new object %s", old_obj->get_base_filename(),
new_obj->get_base_filename());
 
-        if (*new_obj == *old_obj) {
-          Debug("log-config-transfer", "keeping existing object %s", old_obj->get_base_filename());
+      if (*new_obj == *old_obj) {
+        Debug("log-config-transfer", "keeping existing object %s", old_obj->get_base_filename());
 
-          this->_objects[j] = old_obj;
-          old_mgr._objects[i] = NULL;
+        REF_COUNT_OBJ_REFCOUNT_INC(old_obj);
+        this->_objects[j] = old_obj;
 
+        if (REF_COUNT_OBJ_REFCOUNT_DEC(new_obj) == 0) {
           delete new_obj;
-          ++num_kept_objects;
-          break;
         }
+        ++num_kept_objects;
+        break;
       }
     }
   }
 
-  // At this point, the old manager has a sparse object array (ie. some NULL entries). This
is unfortunate, but
-  // we don't want to modify the old manager in a non-atomic way because another thread might
be flushing the
-  // log objects as we do this. We know the manager will be destroyed soon after log object
transfer, so this
-  // intermediate state should not last very long.
-
   if (is_debug_tag_set("log-config-transfer")) {
     Debug("log-config-transfer", "Log Object List after transfer:");
     display();
   }
 }
 
-int
+unsigned
 LogObjectManager::roll_files(long time_now)
 {
   int num_rolled = 0;
-  for (size_t i=0; i < _numObjects; i++) {
-    if (_objects[i]) {
-      num_rolled += _objects[i]->roll_files(time_now);
-    }
+
+  for (unsigned i = 0; i < this->_objects.length(); i++) {
+    num_rolled += this->_objects[i]->roll_files(time_now);
   }
-  for (size_t i=0; i < _numAPIobjects; i++) {
-    if (_APIobjects[i]) {
-      num_rolled += _APIobjects[i]->roll_files(time_now);
-    }
+
+  ACQUIRE_API_MUTEX("A LogObjectManager::roll_files");
+
+  for (unsigned i = 0; i < this->_APIobjects.length(); i++) {
+    num_rolled += this->_APIobjects[i]->roll_files(time_now);
   }
+
+  RELEASE_API_MUTEX("R LogObjectManager::roll_files");
+
   return num_rolled;
 }
 
 void
 LogObjectManager::display(FILE * str)
 {
-  for (size_t i = 0; i < _numObjects; i++) {
-    if (_objects[i]) {
-      _objects[i]->display(str);
-    }
+  for (unsigned i = 0; i < this->_objects.length(); i++) {
+    _objects[i]->display(str);
+  }
+
+  ACQUIRE_API_MUTEX("A LogObjectManager::display");
+  for (unsigned i = 0; i < this->_APIobjects.length(); i++) {
+    _APIobjects[i]->display(str);
   }
+  RELEASE_API_MUTEX("R LogObjectManager::display");
 }
 
 LogObject *
 LogObjectManager::find_by_format_name(const char *name) const
 {
-  for (unsigned i = 0; i < _numObjects; ++i) {
-    if (_objects[i] && _objects[i]->m_format->name_id() == LogFormat::id_from_name(name))
{
-      return _objects[i];
+  for (unsigned i = 0; i < this->_objects.length(); ++i) {
+    if (this->_objects[i] && this->_objects[i]->m_format->name_id() ==
LogFormat::id_from_name(name)) {
+      return this->_objects[i];
     }
   }
   return NULL;
 }
 
-size_t
+unsigned
 LogObjectManager::get_num_collation_clients() const
 {
-  size_t coll_clients = 0;
-  for (unsigned i = 0; i < _numObjects; ++i) {
-    if (_objects[i] && _objects[i]->is_collation_client()) {
+  unsigned coll_clients = 0;
+
+  for (unsigned i = 0; i < this->_objects.length(); ++i) {
+    if (this->_objects[i] && this->_objects[i]->is_collation_client()) {
       ++coll_clients;
     }
   }
@@ -1384,7 +1337,7 @@ LogObjectManager::log(LogAccess * lad)
   int ret = Log::SKIP;
   ProxyMutex *mutex = this_thread()->mutex;
 
-  for (size_t i = 0; i < _numObjects; i++) {
+  for (unsigned i = 0; i < this->_objects.length(); i++) {
     //
     // Auto created LogObject is only applied to LogBuffer
     // data received from network in collation host. It should
@@ -1416,8 +1369,9 @@ LogObjectManager::log(LogAccess * lad)
   } else if (likely(ret & Log::SKIP)) {
     RecIncrRawStat(log_rsb, mutex->thread_holding,
                    log_stat_event_log_access_skip_stat, 1);
-  } else
+  } else {
     ink_release_assert("Unexpected result");
+  }
 
   return ret;
 }
@@ -1425,17 +1379,17 @@ LogObjectManager::log(LogAccess * lad)
 void
 LogObjectManager::flush_all_objects()
 {
-  for (unsigned i = 0; i < this->_numObjects; ++i) {
-    if (this->_objects[i]) {
-      this->_objects[i]->force_new_buffer();
-    }
+  for (unsigned i = 0; i < this->_objects.length(); ++i) {
+    this->_objects[i]->force_new_buffer();
   }
 
-  for (unsigned i = 0; i < this->_numAPIobjects; ++i) {
-    if (this->_APIobjects[i]) {
-      this->_APIobjects[i]->force_new_buffer();
-    }
+  ACQUIRE_API_MUTEX("A LogObjectManager::flush_all_objects");
+
+  for (unsigned i = 0; i < this->_APIobjects.length(); ++i) {
+    this->_APIobjects[i]->force_new_buffer();
   }
+
+  RELEASE_API_MUTEX("R LogObjectManager::flush_all_objects");
 }
 
 #if TS_HAS_TESTS

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/92929698/proxy/logging/LogObject.h
----------------------------------------------------------------------
diff --git a/proxy/logging/LogObject.h b/proxy/logging/LogObject.h
index f13d2fa..07c90e7 100644
--- a/proxy/logging/LogObject.h
+++ b/proxy/logging/LogObject.h
@@ -35,6 +35,7 @@
 #include "LogBuffer.h"
 #include "LogAccess.h"
 #include "LogFilter.h"
+#include "ts/Vec.h"
 
 /*-------------------------------------------------------------------------
   LogObject
@@ -81,7 +82,9 @@ class LogBufferManager
     size_t preproc_buffers(LogBufferSink *sink);
 };
 
-class LogObject
+// LogObject is atomically reference counted, and the reference count is always owned by
+// one or more LogObjectManagers.
+class LogObject : public RefCountObj
 {
 public:
   enum LogObjectFlags
@@ -116,7 +119,7 @@ public:
   int log(LogAccess * lad, const char *text_entry = NULL);
   int va_log(LogAccess * lad, const char * fmt, va_list ap);
 
-  int roll_files(long time_now = 0);
+  unsigned roll_files(long time_now = 0);
 
   int add_to_flush_queue(LogBuffer * buffer)
   {
@@ -230,15 +233,13 @@ private:
   long m_last_roll_time;        // the last time this object rolled
   // its files
 
-  int m_ref_count;
-
   volatile head_p m_log_buffer;     // current work buffer
   unsigned m_buffer_manager_idx;
   LogBufferManager *m_buffer_manager;
 
   void generate_filenames(const char *log_dir, const char *basename, LogFileFormat file_format);
   void _setup_rolling(Log::RollingEnabledValues rolling_enabled, int rolling_interval_sec,
int rolling_offset_hr, int rolling_size_mb);
-  int _roll_files(long interval_start, long interval_end);
+  unsigned _roll_files(long interval_start, long interval_end);
 
   LogBuffer *_checkout_write(size_t * write_offset, size_t write_size);
 
@@ -265,6 +266,8 @@ public:
 
   inkcoreapi int write(const char *format, ...) TS_PRINTFLIKE(2, 3);
   inkcoreapi int va_write(const char *format, va_list ap);
+
+  static const LogFormat * textfmt;
 };
 
 /*-------------------------------------------------------------------------
@@ -310,28 +313,19 @@ public:
 
 private:
 
-  LogObject ** _objects;      // array of objects managed
-  size_t _numObjects;           // the number of objects managed
-  size_t _maxObjects;           // the maximum capacity of the array
-  // of objects managed
+  typedef Vec<LogObject *> LogObjectList;
 
-  LogObject **_APIobjects;      // array of API objects
-  size_t _numAPIobjects;        // the number of API objects managed
-  size_t _maxAPIobjects;        // the maximum capacity of the array
-  // of API objects managed
+  LogObjectList _objects;         // array of configured objects
+  LogObjectList _APIobjects;      // array of API objects
 
 public:
-    ink_mutex * _APImutex;      // synchronize access to array of API
-  // objects
+    ink_mutex * _APImutex;      // synchronize access to array of API objects
 private:
 
   int _manage_object(LogObject * log_object, bool is_api_object, int maxConflicts);
-  static bool _has_internal_filename_conflict(const char *filename, LogObject ** objects,
int numObjects);
+  static bool _has_internal_filename_conflict(const char *filename, LogObjectList& objects);
   int _solve_filename_conflicts(LogObject * log_obj, int maxConflicts);
   int _solve_internal_filename_conflicts(LogObject * log_obj, int maxConflicts, int fileNum
= 0);
-  void _add_object(LogObject * object);
-  void _add_api_object(LogObject * object);
-  int _roll_files(long time_now, bool roll_only_if_needed);
 
 public:
   LogObjectManager();
@@ -357,9 +351,8 @@ public:
 
   LogObject *get_object_with_signature(uint64_t signature);
   void check_buffer_expiration(long time_now);
-  size_t get_num_objects() const { return _numObjects; }
 
-  int roll_files(long time_now);
+  unsigned roll_files(long time_now);
 
   int log(LogAccess * lad);
   void display(FILE * str = stdout);
@@ -369,9 +362,9 @@ public:
   void open_local_pipes();
   void transfer_objects(LogObjectManager & mgr);
 
-  bool has_api_objects() const  { return (_numAPIobjects > 0); }
-
-  size_t get_num_collation_clients() const;
+  bool has_api_objects() const  { return _APIobjects.length() > 0; }
+  unsigned get_num_objects() const { return _objects.length(); }
+  unsigned get_num_collation_clients() const;
 };
 
 inline bool


Mime
View raw message