shill: fix DisableTechnology with multiple devices of a technology
For some Devices (e.g. Cellular), EnableTechnology and DisableTechnology are
asynchronous operations. Previously, shill would invoke the completion
callback for these operations, as each Device completed its work.
Unsurprisingly, this would fail on the second device.
Fix this by introducing ResultAggregator, which aggregates the results from
multiple asynchronous operations. Now, if the Devices process
EnableTechnology or DisableTechnology asynchronously, then we will return
only after the final Device has completed the request.
While there: merge the code for EnableTechnology and DisableTechnology,
since the methods were symmetric, but non-trivial.
Note that one ugly case remains: if one or more Devices fails immediately,
but others all succeed asynchronously, we will not report any failure.
BUG=chromium:258206
TEST=new unit tests, manual
Manual testing
--------------
- grab a device with two cellular modems (e.g. on-board and usb)
- disable cellular from the ash tray
-> shill should not crash
- enable cellular from the ash tray
-> shill should not crash
Change-Id: I09f94326342900e4cee6929d9edbe5cf735c92d7
Reviewed-on: https://gerrit.chromium.org/gerrit/62016
Commit-Queue: mukesh agrawal <quiche@chromium.org>
Reviewed-by: mukesh agrawal <quiche@chromium.org>
Tested-by: mukesh agrawal <quiche@chromium.org>
diff --git a/result_aggregator_unittest.cc b/result_aggregator_unittest.cc
new file mode 100644
index 0000000..11cfd49
--- /dev/null
+++ b/result_aggregator_unittest.cc
@@ -0,0 +1,88 @@
+// Copyright (c) 2013 The Chromium OS Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "shill/result_aggregator.h"
+
+#include <base/bind.h>
+#include <base/memory/ref_counted.h>
+#include <gmock/gmock.h>
+#include <gtest/gtest.h>
+
+namespace shill {
+
+using base::Bind;
+using base::Unretained;
+
+class ResultAggregatorTest : public ::testing::Test {
+ public:
+ ResultAggregatorTest()
+ : aggregator_(new ResultAggregator(
+ Bind(&ResultAggregatorTest::ReportResult, Unretained(this)))) {}
+ virtual ~ResultAggregatorTest() {}
+
+ virtual void TearDown() {
+ aggregator_ = NULL; // Ensure ReportResult is invoked before our dtor.
+ }
+
+ protected:
+ MOCK_METHOD1(ReportResult, void(const Error &));
+ scoped_refptr<ResultAggregator> aggregator_;
+};
+
+class ResultGenerator {
+ public:
+ explicit ResultGenerator(const scoped_refptr<ResultAggregator> &aggregator)
+ : aggregator_(aggregator) {}
+ ~ResultGenerator() {}
+
+ void GenerateResult(const Error::Type error_type) {
+ aggregator_->ReportResult(Error(error_type));
+ }
+
+ private:
+ scoped_refptr<ResultAggregator> aggregator_;
+ DISALLOW_COPY_AND_ASSIGN(ResultGenerator);
+};
+
+MATCHER_P(ErrorType, type, "") {
+ return arg.type() == type;
+}
+
+TEST_F(ResultAggregatorTest, Unused) {
+ EXPECT_CALL(*this, ReportResult(ErrorType(Error::kSuccess))).Times(0);
+}
+
+TEST_F(ResultAggregatorTest, BothSucceed) {
+ EXPECT_CALL(*this, ReportResult(ErrorType(Error::kSuccess)));
+ ResultGenerator first_generator(aggregator_);
+ ResultGenerator second_generator(aggregator_);
+ first_generator.GenerateResult(Error::kSuccess);
+ second_generator.GenerateResult(Error::kSuccess);
+}
+
+TEST_F(ResultAggregatorTest, FirstFails) {
+ EXPECT_CALL(*this, ReportResult(ErrorType(Error::kOperationTimeout)));
+ ResultGenerator first_generator(aggregator_);
+ ResultGenerator second_generator(aggregator_);
+ first_generator.GenerateResult(Error::kOperationTimeout);
+ second_generator.GenerateResult(Error::kSuccess);
+}
+
+TEST_F(ResultAggregatorTest, SecondFails) {
+ EXPECT_CALL(*this, ReportResult(ErrorType(Error::kOperationTimeout)));
+ ResultGenerator first_generator(aggregator_);
+ ResultGenerator second_generator(aggregator_);
+ first_generator.GenerateResult(Error::kSuccess);
+ second_generator.GenerateResult(Error::kOperationTimeout);
+}
+
+TEST_F(ResultAggregatorTest, BothFail) {
+ EXPECT_CALL(*this, ReportResult(ErrorType(Error::kOperationTimeout)));
+ ResultGenerator first_generator(aggregator_);
+ ResultGenerator second_generator(aggregator_);
+ first_generator.GenerateResult(Error::kOperationTimeout);
+ second_generator.GenerateResult(Error::kPermissionDenied);
+}
+
+} // namespace shill