Use the Guard when testing bundle pool

3 files changed, 31 insertions, 33 deletions

diff --git a/vespalib/src/tests/simple_thread_bundle/simple_thread_bundle_test.cpp b/vespalib/src/tests/simple_thread_bundle/simple_thread_bundle_test.cpp
index e451f1e033d..b155bb02d42 100644
--- a/vespalib/src/tests/simple_thread_bundle/simple_thread_bundle_test.cpp
+++ b/vespalib/src/tests/simple_thread_bundle/simple_thread_bundle_test.cpp
@@ -160,15 +160,11 @@ TEST("require that all strategies work with variable number of threads and targe
 }
 
 TEST_F("require that bundle pool gives out bundles", SimpleThreadBundle::Pool(5)) {
-    SimpleThreadBundle::UP b1 = f1.obtain();
-    SimpleThreadBundle::UP b2 = f1.obtain();
-    ASSERT_TRUE(b1.get() != 0);
-    ASSERT_TRUE(b2.get() != 0);
-    EXPECT_EQUAL(5u, b1->size());
-    EXPECT_EQUAL(5u, b2->size());
-    EXPECT_FALSE(b1.get() == b2.get());
-    f1.release(std::move(b1));
-    f1.release(std::move(b2));
+    auto b1 = f1.getBundle();
+    auto b2 = f1.getBundle();
+    EXPECT_EQUAL(5u, b1.bundle().size());
+    EXPECT_EQUAL(5u, b2.bundle().size());
+    EXPECT_FALSE(&b1.bundle() == &b2.bundle());
 }
 
 TEST_F("require that bundles do not need to be put back on the pool", SimpleThreadBundle::Pool(5)) {
@@ -178,20 +174,20 @@ TEST_F("require that bundles do not need to be put back on the pool", SimpleThre
 }
 
 TEST_F("require that bundle pool reuses bundles", SimpleThreadBundle::Pool(5)) {
-    SimpleThreadBundle::UP bundle = f1.obtain();
-    SimpleThreadBundle *ptr = bundle.get();
-    f1.release(std::move(bundle));
-    bundle = f1.obtain();
-    EXPECT_EQUAL(ptr, bundle.get());
+    SimpleThreadBundle *ptr;
+    {
+        ptr = &f1.getBundle().bundle();
+    }
+    auto bundle = f1.getBundle();
+    EXPECT_EQUAL(ptr, &bundle.bundle());
 }
 
 TEST_MT_FF("require that bundle pool works with multiple threads", 32, SimpleThreadBundle::Pool(3),
            std::vector<SimpleThreadBundle*>(num_threads, 0))
 {
-    SimpleThreadBundle::UP bundle = f1.obtain();
-    ASSERT_TRUE(bundle.get() != 0);
-    EXPECT_EQUAL(3u, bundle->size());
-    f2[thread_id] = bundle.get();
+    SimpleThreadBundle::Pool::Guard bundle = f1.getBundle();
+    EXPECT_EQUAL(3u, bundle.bundle().size());
+    f2[thread_id] = &bundle.bundle();
     TEST_BARRIER();
     if (thread_id == 0) {
         for (size_t i = 0; i < num_threads; ++i) {
@@ -201,13 +197,12 @@ TEST_MT_FF("require that bundle pool works with multiple threads", 32, SimpleThr
         }
     }
     TEST_BARRIER();
-    f1.release(std::move(bundle));
 }
 
 struct Filler {
     int stuff;
     Filler() : stuff(0) {}
-    virtual ~Filler() {}
+    virtual ~Filler() = default;
 };
 
 struct Proxy : Filler, Runnable {
diff --git a/vespalib/src/vespa/vespalib/util/simple_thread_bundle.cpp b/vespalib/src/vespa/vespalib/util/simple_thread_bundle.cpp
index 958bb58f34a..4a588709c60 100644
--- a/vespalib/src/vespa/vespalib/util/simple_thread_bundle.cpp
+++ b/vespalib/src/vespa/vespalib/util/simple_thread_bundle.cpp
@@ -87,7 +87,7 @@ SimpleThreadBundle::Pool::obtain()
             return ret;
         }
     }
-    return std::make_unique<SimpleThreadBundle>(_bundleSize, _init_fun);
+    return std::make_unique<SimpleThreadBundle>(_bundleSize, _init_fun, USE_SIGNAL_LIST);
 }
 
 void
@@ -142,11 +142,11 @@ SimpleThreadBundle::SimpleThreadBundle(size_t size_in, Runnable::init_fun_t init
 
 SimpleThreadBundle::~SimpleThreadBundle()
 {
-    for (size_t i = 0; i < _signals.size(); ++i) {
-        _signals[i].cancel();
+    for (auto & _signal : _signals) {
+        _signal.cancel();
     }
-    for (size_t i = 0; i < _workers.size(); ++i) {
-        _workers[i]->thread.join();
+    for (const auto & _worker : _workers) {
+        _worker->thread.join();
     }
 }
 
diff --git a/vespalib/src/vespa/vespalib/util/simple_thread_bundle.h b/vespalib/src/vespa/vespalib/util/simple_thread_bundle.h
index 70dbfbffb84..5dde0575a74 100644
--- a/vespalib/src/vespa/vespalib/util/simple_thread_bundle.h
+++ b/vespalib/src/vespa/vespalib/util/simple_thread_bundle.h
@@ -21,7 +21,7 @@ struct Work {
     Runnable* const* targets;
     size_t cnt;
     CountDownLatch *latch;
-    Work() : targets(nullptr), cnt(0), latch(0) {}
+    Work() : targets(nullptr), cnt(0), latch(nullptr) {}
 };
 
 /**
@@ -31,7 +31,7 @@ struct Part {
     const Work &work;
     size_t offset;
     Part(const Work &w, size_t o) : work(w), offset(o) {}
-    bool valid() { return (offset < work.cnt); }
+    bool valid() const noexcept { return (offset < work.cnt); }
     void perform() {
         if (valid()) {
             work.targets[offset]->run();
@@ -51,7 +51,7 @@ struct Signal {
     Signal() noexcept;
     Signal(Signal &&) noexcept = default;
     ~Signal();
-    size_t wait(size_t &localGen) {
+    size_t wait(size_t &localGen) const {
         std::unique_lock guard(*monitor);
         while (localGen == generation) {
             cond->wait(guard);
@@ -105,7 +105,7 @@ public:
     public:
         class Guard {
         public:
-            Guard(Pool & pool) : _bundle(pool.obtain()), _pool(pool) {}
+            explicit Guard(Pool & pool) : _bundle(pool.obtain()), _pool(pool) {}
             Guard(const Guard &) = delete;
             Guard & operator =(const Guard &) = delete;
             ~Guard() { _pool.release(std::move(_bundle)); }
@@ -115,9 +115,10 @@ public:
             Pool                   &_pool;
         };
         Pool(size_t bundleSize, init_fun_t init_fun);
-        Pool(size_t bundleSize) : Pool(bundleSize, Runnable::default_init_function) {}
+        explicit Pool(size_t bundleSize) : Pool(bundleSize, Runnable::default_init_function) {}
         ~Pool();
         Guard getBundle() { return Guard(*this); }
+        //TODO Make private
         SimpleThreadBundle::UP obtain();
         void release(SimpleThreadBundle::UP bundle);
     };
@@ -138,10 +139,12 @@ private:
     Runnable::UP            _hook;
 
 public:
-    SimpleThreadBundle(size_t size, init_fun_t init_fun, Strategy strategy = USE_SIGNAL_LIST);
-    SimpleThreadBundle(size_t size, Strategy strategy = USE_SIGNAL_LIST)
+    SimpleThreadBundle(size_t size, init_fun_t init_fun, Strategy strategy);
+    SimpleThreadBundle(size_t size, Strategy strategy)
       : SimpleThreadBundle(size, Runnable::default_init_function, strategy) {}
-    ~SimpleThreadBundle();
+    explicit SimpleThreadBundle(size_t size)
+            : SimpleThreadBundle(size, USE_SIGNAL_LIST) {}
+    ~SimpleThreadBundle() override;
     size_t size() const override;
     using ThreadBundle::run;
     void run(Runnable* const* targets, size_t cnt) override;