Fix for Issue 296 and Issue 297 in branch 17.
diff --git a/src/net/instaweb/apache/apache_rewrite_driver_factory.cc b/src/net/instaweb/apache/apache_rewrite_driver_factory.cc
index d54e051..86b7054 100644
--- a/src/net/instaweb/apache/apache_rewrite_driver_factory.cc
+++ b/src/net/instaweb/apache/apache_rewrite_driver_factory.cc
@@ -57,6 +57,7 @@
version_(version.data(), version.size()),
statistics_enabled_(true),
statistics_frozen_(false),
+ owns_statistics_(false),
test_proxy_(false),
is_root_process_(true),
use_shared_mem_locking_(false),
@@ -225,7 +226,7 @@
cache_mutex_.reset(NULL);
rewrite_drivers_mutex_.reset(NULL);
if (is_root_process_) {
- if (shared_mem_statistics_ != NULL) {
+ if (owns_statistics_ && (shared_mem_statistics_ != NULL)) {
shared_mem_statistics_->GlobalCleanup(message_handler());
}
shared_mem_lock_manager_lifecycler_.GlobalCleanup(message_handler());
diff --git a/src/net/instaweb/apache/apache_rewrite_driver_factory.h b/src/net/instaweb/apache/apache_rewrite_driver_factory.h
index 9fdec04..478da29 100644
--- a/src/net/instaweb/apache/apache_rewrite_driver_factory.h
+++ b/src/net/instaweb/apache/apache_rewrite_driver_factory.h
@@ -85,6 +85,7 @@
virtual Statistics* statistics();
void SetStatistics(SharedMemStatistics* x);
void set_statistics_enabled(bool x) { statistics_enabled_ = x; }
+ void set_owns_statistics(bool o) { owns_statistics_ = o; }
bool statistics_enabled() const { return statistics_enabled_; }
AbstractSharedMem* shared_mem_runtime() const {
@@ -176,6 +177,9 @@
std::string version_;
bool statistics_enabled_;
bool statistics_frozen_;
+ bool owns_statistics_; // If true, this particular factory is responsible
+ // for calling GlobalCleanup on the (global)
+ // statistics object (but not delete'ing it)
bool test_proxy_;
bool is_root_process_;
diff --git a/src/net/instaweb/apache/mod_instaweb.cc b/src/net/instaweb/apache/mod_instaweb.cc
index 4c63f66..61bbeea 100644
--- a/src/net/instaweb/apache/mod_instaweb.cc
+++ b/src/net/instaweb/apache/mod_instaweb.cc
@@ -283,7 +283,8 @@
: merge_time_us_(NULL),
parse_time_us_(NULL),
html_rewrite_time_us_(NULL),
- stored_parse_time_us_(0) {
+ stored_parse_time_us_(0),
+ all_factories_cleared_(false) {
}
~ApacheProcessContext() {
@@ -303,6 +304,9 @@
// is being deleted on a pool hook.
void remove_factory(ApacheRewriteDriverFactory* factory) {
factories_.erase(factory);
+ if (factories_.empty()) {
+ all_factories_cleared_ = true;
+ }
}
// Delete the specified config on process exit.
@@ -316,21 +320,30 @@
configs_.erase(config);
}
- SharedMemStatistics* InitStatistics(AbstractSharedMem* shmem_runtime,
- const StringPiece& filename_prefix,
- MessageHandler* message_handler) {
+ // Initializes global statistics object if needed, using factory to
+ // help with the settings if needed.
+ // Note: does not call set_statistics() on the factory.
+ SharedMemStatistics* InitStatistics(ApacheRewriteDriverFactory* factory) {
+ if (all_factories_cleared_) {
+ // Get rid of stale instance from configuration check.
+ all_factories_cleared_ = false;
+ statistics_.reset(NULL);
+ }
+
if (statistics_.get() == NULL) {
// Note that we create the statistics object in the parent process, and
// it stays around in the kids but gets reinitialized for them
// with a call to InitVariables(false) inside pagespeed_child_init.
+ factory->set_owns_statistics(true);
statistics_.reset(
- new SharedMemStatistics(shmem_runtime, filename_prefix.as_string()));
+ new SharedMemStatistics(factory->shared_mem_runtime(),
+ factory->filename_prefix().as_string()));
ResourceManager::Initialize(statistics_.get());
SerfUrlAsyncFetcher::Initialize(statistics_.get());
statistics_->AddVariable("merge_time_us");
statistics_->AddVariable("parse_time_us");
statistics_->AddVariable("html_rewrite_time_us");
- statistics_->InitVariables(true, message_handler);
+ statistics_->InitVariables(true, factory->message_handler());
merge_time_us_ = statistics_->GetVariable("merge_time_us");
parse_time_us_ = statistics_->GetVariable("parse_time_us");
html_rewrite_time_us_ = statistics_->GetVariable("html_rewrite_time_us");
@@ -375,6 +388,13 @@
Variable* parse_time_us_;
Variable* html_rewrite_time_us_;
int64 stored_parse_time_us_;
+
+ // This variable is used to detect us retaining some state from between
+ // the configuration check and configuration parse; if we see all factories
+ // be destroyed while we remain it means Apache proceeded from config check
+ // to actual config parse + startup without ~ApacheProcessContext being
+ // called.
+ bool all_factories_cleared_;
};
ApacheProcessContext apache_process_context;
@@ -713,12 +733,6 @@
return HTTP_INTERNAL_SERVER_ERROR;
}
- if ((factory->statistics_enabled() && (statistics == NULL))) {
- statistics = apache_process_context.InitStatistics(
- factory->shared_mem_runtime(), factory->filename_prefix(),
- factory->message_handler());
- }
-
// If we are running as root, hand over the ownership of data directories
// we made to the eventual Apache uid/gid.
// (Apache will not switch from current euid otherwise --- see
@@ -734,8 +748,13 @@
}
}
}
- }
+ // See if we need a statistics object. We may need that even when
+ // we are disabled, in case we get turned on via .htaccess or query param.
+ if (factory->statistics_enabled() && (statistics == NULL)) {
+ statistics = apache_process_context.InitStatistics(factory);
+ }
+ }
// Next we do the instance-independent static initialization, once we have
// established whether *any* of the servers have stats enabled.
ResourceManager::Initialize(statistics);
