Merge "metricsd: Cleanup on TERM signal."

am: 90bfc8035b

* commit '90bfc8035b8a188c0af9f74ad54b73bf8ac198f4':
  metricsd: Cleanup on TERM signal.

metricsd/Android.mk

@@ -38,6 +38,7 @@ metricsd_common := \
   uploader/metrics_hashes.cc \
   uploader/metrics_log_base.cc \
   uploader/metrics_log.cc \
+  uploader/metricsd_service_runner.cc \
   uploader/sender_http.cc \
   uploader/system_profile_cache.cc \
   uploader/upload_service.cc
@@ -84,6 +85,7 @@ metrics_collector_static_libraries := libmetricscollectorservice
 metricsd_shared_libraries := \
   libbinder \
   libbrillo \
+  libbrillo-binder \
   libbrillo-http \
   libchrome \
   libprotobuf-cpp-lite \

metricsd/metricsd_main.cc

@@ -14,21 +14,15 @@
  * limitations under the License.
  */
 
-#include <thread>
-
-#include <base/at_exit.h>
 #include <base/command_line.h>
 #include <base/files/file_path.h>
 #include <base/logging.h>
-#include <base/metrics/statistics_recorder.h>
-#include <base/strings/string_util.h>
 #include <base/time/time.h>
 #include <brillo/flag_helper.h>
 #include <brillo/syslog_logging.h>
 
 #include "constants.h"
-#include "uploader/bn_metricsd_impl.h"
-#include "uploader/crash_counters.h"
+#include "uploader/metricsd_service_runner.h"
 #include "uploader/upload_service.h"
 
 int main(int argc, char** argv) {
@@ -76,18 +70,10 @@ int main(int argc, char** argv) {
     return errno;
   }
 
-  std::shared_ptr<CrashCounters> counters(new CrashCounters);
-
   UploadService upload_service(
       FLAGS_server, base::TimeDelta::FromSeconds(FLAGS_upload_interval_secs),
       base::FilePath(FLAGS_private_directory),
-      base::FilePath(FLAGS_shared_directory), counters);
+      base::FilePath(FLAGS_shared_directory));
 
-  base::StatisticsRecorder::Initialize();
-
-  // Create and start the binder thread.
-  BnMetricsdImpl binder_service(counters);
-  std::thread binder_thread(&BnMetricsdImpl::Run, &binder_service);
-
-  upload_service.Run();
+  return upload_service.Run();
 }

metricsd/uploader/bn_metricsd_impl.cc

@@ -19,8 +19,6 @@
 #include <base/metrics/histogram.h>
 #include <base/metrics/sparse_histogram.h>
 #include <base/metrics/statistics_recorder.h>
-#include <binder/IPCThreadState.h>
-#include <binder/IServiceManager.h>
 #include <utils/Errors.h>
 #include <utils/String16.h>
 #include <utils/String8.h>
@@ -37,16 +35,6 @@ BnMetricsdImpl::BnMetricsdImpl(const std::shared_ptr<CrashCounters>& counters)
   CHECK(counters_) << "Invalid counters argument to constructor";
 }
 
-void BnMetricsdImpl::Run() {
-  android::status_t status =
-      android::defaultServiceManager()->addService(getInterfaceDescriptor(),
-                                                   this);
-  CHECK(status == android::OK) << "Metricsd service registration failed";
-  android::ProcessState::self()->setThreadPoolMaxThreadCount(0);
-  android::IPCThreadState::self()->disableBackgroundScheduling(true);
-  android::IPCThreadState::self()->joinThreadPool();
-}
-
 Status BnMetricsdImpl::recordHistogram(
     const String16& name, int sample, int min, int max, int nbuckets) {
   base::HistogramBase* histogram = base::Histogram::FactoryGet(

metricsd/uploader/bn_metricsd_impl.h

@@ -25,9 +25,6 @@ class BnMetricsdImpl : public android::brillo::metrics::BnMetricsd {
   explicit BnMetricsdImpl(const std::shared_ptr<CrashCounters>& counters);
   virtual ~BnMetricsdImpl() = default;
 
-  // Starts the binder main loop.
-  void Run();
-
   // Records a histogram.
   android::binder::Status recordHistogram(const android::String16& name,
                                           int sample,

metricsd/uploader/metricsd_service_runner.cc

@@ -0,0 +1,60 @@
+/*
+ * Copyright (C) 2015 The Android Open Source Project
+ *
+ * Licensed 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.
+ */
+
+#include "uploader/metricsd_service_runner.h"
+
+#include <thread>
+
+#include <binder/IServiceManager.h>
+#include <brillo/binder_watcher.h>
+#include <utils/Errors.h>
+
+#include "uploader/bn_metricsd_impl.h"
+
+MetricsdServiceRunner::MetricsdServiceRunner(
+    std::shared_ptr<CrashCounters> counters)
+    : counters_(counters) {}
+
+void MetricsdServiceRunner::Start() {
+  thread_.reset(new std::thread(&MetricsdServiceRunner::Run, this));
+}
+
+void MetricsdServiceRunner::Run() {
+  android::sp<BnMetricsdImpl> metrics_service(new BnMetricsdImpl(counters_));
+
+  android::status_t status = android::defaultServiceManager()->addService(
+      metrics_service->getInterfaceDescriptor(), metrics_service);
+  CHECK(status == android::OK) << "Metricsd service registration failed";
+
+  message_loop_for_io_.reset(new base::MessageLoopForIO);
+
+  brillo::BinderWatcher watcher;
+  CHECK(watcher.Init()) << "failed to initialize the binder file descriptor "
+                        << "watcher";
+
+  message_loop_for_io_->Run();
+
+  // Delete the message loop here as it needs to be deconstructed in the thread
+  // it is attached to.
+  message_loop_for_io_.reset();
+}
+
+void MetricsdServiceRunner::Stop() {
+  message_loop_for_io_->PostTask(FROM_HERE,
+                                 message_loop_for_io_->QuitClosure());
+
+  thread_->join();
+}

metricsd/uploader/metricsd_service_runner.h

@@ -0,0 +1,47 @@
+/*
+ * Copyright (C) 2015 The Android Open Source Project
+ *
+ * Licensed 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.
+ */
+
+#ifndef METRICS_UPLOADER_METRISCD_SERVICE_RUNNER_H_
+#define METRICS_UPLOADER_METRISCD_SERVICE_RUNNER_H_
+
+#include <memory>
+#include <thread>
+
+#include <base/message_loop/message_loop.h>
+
+#include "uploader/crash_counters.h"
+
+class MetricsdServiceRunner {
+ public:
+  MetricsdServiceRunner(std::shared_ptr<CrashCounters> counters);
+
+  // Start the Metricsd Binder service in a new thread.
+  void Start();
+
+  // Stop the Metricsd service and wait for its thread to exit.
+  void Stop();
+
+ private:
+  // Creates and run the main loop for metricsd's Binder service.
+  void Run();
+
+  std::unique_ptr<base::MessageLoopForIO> message_loop_for_io_;
+
+  std::unique_ptr<std::thread> thread_;
+  std::shared_ptr<CrashCounters> counters_;
+};
+
+#endif  // METRICS_UPLOADER_METRISCD_SERVICE_RUNNER_H_

metricsd/uploader/upload_service.cc

@@ -43,19 +43,25 @@ const int UploadService::kMaxFailedUpload = 10;
 UploadService::UploadService(const std::string& server,
                              const base::TimeDelta& upload_interval,
                              const base::FilePath& private_metrics_directory,
-                             const base::FilePath& shared_metrics_directory,
-                             const std::shared_ptr<CrashCounters> counters)
-    : histogram_snapshot_manager_(this),
+                             const base::FilePath& shared_metrics_directory)
+    : brillo::Daemon(),
+      histogram_snapshot_manager_(this),
       sender_(new HttpSender(server)),
       failed_upload_count_(metrics::kFailedUploadCountName,
                            private_metrics_directory),
-      counters_(counters),
-      upload_interval_(upload_interval) {
+      counters_(new CrashCounters),
+      upload_interval_(upload_interval),
+      metricsd_service_runner_(counters_) {
   staged_log_path_ = private_metrics_directory.Append(metrics::kStagedLogName);
   consent_file_ = shared_metrics_directory.Append(metrics::kConsentFileName);
 }
 
 int UploadService::OnInit() {
+  brillo::Daemon::OnInit();
+
+  base::StatisticsRecorder::Initialize();
+  metricsd_service_runner_.Start();
+
   system_profile_setter_.reset(new SystemProfileCache());
 
   base::MessageLoop::current()->PostDelayedTask(FROM_HERE,
@@ -63,9 +69,14 @@ int UploadService::OnInit() {
                  base::Unretained(this),
                  upload_interval_),
       upload_interval_);
+
   return EX_OK;
 }
 
+void UploadService::OnShutdown(int* exit_code) {
+  metricsd_service_runner_.Stop();
+}
+
 void UploadService::InitForTest(SystemProfileSetter* setter) {
   system_profile_setter_.reset(setter);
 }

metricsd/uploader/upload_service.h

@@ -28,6 +28,7 @@
 #include "persistent_integer.h"
 #include "uploader/crash_counters.h"
 #include "uploader/metrics_log.h"
+#include "uploader/metricsd_service_runner.h"
 #include "uploader/proto/chrome_user_metrics_extension.pb.h"
 #include "uploader/sender.h"
 #include "uploader/system_profile_cache.h"
@@ -66,11 +67,13 @@ class UploadService : public base::HistogramFlattener, public brillo::Daemon {
   UploadService(const std::string& server,
                 const base::TimeDelta& upload_interval,
                 const base::FilePath& private_metrics_directory,
-                const base::FilePath& shared_metrics_directory,
-                const std::shared_ptr<CrashCounters> counters);
+                const base::FilePath& shared_metrics_directory);
 
   // Initializes the upload service.
-  int OnInit();
+  int OnInit() override;
+
+  // Cleans up the internal state before exiting.
+  void OnShutdown(int* exit_code) override;
 
   // Starts a new log. The log needs to be regenerated after each successful
   // launch as it is destroyed when staging the log.
@@ -154,6 +157,8 @@ class UploadService : public base::HistogramFlattener, public brillo::Daemon {
 
   base::TimeDelta upload_interval_;
 
+  MetricsdServiceRunner metricsd_service_runner_;
+
   base::FilePath consent_file_;
   base::FilePath staged_log_path_;
 

metricsd/uploader/upload_service_test.cc

@@ -53,10 +53,10 @@ class UploadServiceTest : public testing::Test {
 
     ASSERT_EQ(0, base::WriteFile(shared_dir.Append(metrics::kConsentFileName),
                                  "", 0));
-    counters_.reset(new CrashCounters);
 
-    upload_service_.reset(new UploadService("", base::TimeDelta(), private_dir,
-                                            shared_dir, counters_));
+    upload_service_.reset(
+        new UploadService("", base::TimeDelta(), private_dir, shared_dir));
+    counters_ = upload_service_->counters_;
 
     upload_service_->sender_.reset(new SenderMock);
     upload_service_->InitForTest(new MockSystemProfileSetter);
@@ -149,12 +149,9 @@ TEST_F(UploadServiceTest, EmptyLogsAreNotSent) {
 }
 
 TEST_F(UploadServiceTest, LogEmptyByDefault) {
-  UploadService upload_service("", base::TimeDelta(), dir_.path(), dir_.path(),
-                               std::make_shared<CrashCounters>());
-
-  // current_log_ should be initialized later as it needs AtExitManager to exit
+  // current_log_ should be initialized later as it needs AtExitManager to exist
   // in order to gather system information from SysInfo.
-  EXPECT_FALSE(upload_service.current_log_);
+  EXPECT_FALSE(upload_service_->current_log_);
 }
 
 TEST_F(UploadServiceTest, CanSendMultipleTimes) {