Only reset when nothing has changed and the whole changeset has been consumed.

5 files changed, 33 insertions, 25 deletions

diff --git a/config/src/tests/subscriber/subscriber.cpp b/config/src/tests/subscriber/subscriber.cpp
index 7613ba41234..39a537486cd 100644
--- a/config/src/tests/subscriber/subscriber.cpp
+++ b/config/src/tests/subscriber/subscriber.cpp
@@ -341,22 +341,22 @@ TEST_FF("requireThatHandlesAreMarkedAsChanged", MyManager, APIFixture(f1)) {
     ConfigSubscriber s(IConfigContext::SP(new APIFixture(f2)));
     ConfigHandle<FooConfig>::UP h1 = s.subscribe<FooConfig>("myid2");
     ConfigHandle<BarConfig>::UP h2 = s.subscribe<BarConfig>("myid2");
-    ASSERT_FALSE(s.nextConfig(0));
+    EXPECT_FALSE(s.nextConfig(0));
 
     f1.updateValue(0, createFooValue("foo"), 1);
     f1.updateValue(1, createFooValue("bar"), 1);
-    ASSERT_TRUE(s.nextConfig(100));
-    ASSERT_TRUE(h1->isChanged());
-    ASSERT_TRUE(h2->isChanged());
+    EXPECT_TRUE(s.nextConfig(100));
+    EXPECT_TRUE(h1->isChanged());
+    EXPECT_TRUE(h2->isChanged());
 
-    ASSERT_FALSE(s.nextConfig(100));
-    ASSERT_FALSE(h1->isChanged());
-    ASSERT_FALSE(h2->isChanged());
+    EXPECT_FALSE(s.nextConfig(100));
+    EXPECT_FALSE(h1->isChanged());
+    EXPECT_FALSE(h2->isChanged());
     f1.updateValue(0, createFooValue("bar"), 2);
     f1.updateGeneration(1, 2);
-    ASSERT_TRUE(s.nextConfig(100));
-    ASSERT_TRUE(h1->isChanged());
-    ASSERT_FALSE(h2->isChanged());
+    EXPECT_TRUE(s.nextConfig(100));
+    EXPECT_TRUE(h1->isChanged());
+    EXPECT_FALSE(h2->isChanged());
 }
 
 TEST_FF("requireThatNextGenerationMarksChanged", MyManager, APIFixture(f1)) {
diff --git a/config/src/tests/subscription/subscription.cpp b/config/src/tests/subscription/subscription.cpp
index 5d36753813c..a9e4c923e92 100644
--- a/config/src/tests/subscription/subscription.cpp
+++ b/config/src/tests/subscription/subscription.cpp
@@ -101,11 +101,11 @@ TEST_MT_F("requireThatNextUpdateReturnsInterrupted", 2, SubscriptionFixture(Conf
 
 TEST_F("Require that isChanged takes generation into account", SubscriptionFixture(ConfigKey::create<MyConfig>("myid")))
 {
-    f1.holder->handle(ConfigUpdate::UP(new ConfigUpdate(ConfigValue(), true, 1)));
+    f1.holder->handle(ConfigUpdate::UP(new ConfigUpdate(ConfigValue(std::vector<vespalib::string>(), "a"), true, 1)));
     ASSERT_TRUE(f1.sub.nextUpdate(0, 0));
     f1.sub.flip();
     ASSERT_EQUAL(1, f1.sub.getLastGenerationChanged());
-    f1.holder->handle(ConfigUpdate::UP(new ConfigUpdate(ConfigValue(), true, 2)));
+    f1.holder->handle(ConfigUpdate::UP(new ConfigUpdate(ConfigValue(std::vector<vespalib::string>(), "b"), true, 2)));
     ASSERT_TRUE(f1.sub.nextUpdate(1, 0));
     f1.sub.flip();
     ASSERT_EQUAL(2, f1.sub.getLastGenerationChanged());
diff --git a/config/src/vespa/config/subscription/configsubscription.cpp b/config/src/vespa/config/subscription/configsubscription.cpp
index b79e3dab1fd..efa3f5a02e3 100644
--- a/config/src/vespa/config/subscription/configsubscription.cpp
+++ b/config/src/vespa/config/subscription/configsubscription.cpp
@@ -28,8 +28,9 @@ ConfigSubscription::~ConfigSubscription()
 bool
 ConfigSubscription::nextUpdate(int64_t generation, uint64_t timeoutInMillis)
 {
-    if (_closed || !_holder->poll())
+    if (_closed || !_holder->poll()) {
         return false;
+    }
     _next = _holder->provide();
     if (isGenerationNewer(_next->getGeneration(), generation)) {
         return true;
@@ -38,9 +39,15 @@ ConfigSubscription::nextUpdate(int64_t generation, uint64_t timeoutInMillis)
 }
 
 bool
+ConfigSubscription::hasGenerationChanged() const
+{
+    return (!_closed && _next && ((_current && (_current->getGeneration() != _next->getGeneration())) || ! _current));
+}
+
+bool
 ConfigSubscription::hasChanged() const
 {
-    return (!_closed && (_next->hasChanged() || _current.get() == NULL));
+    return (!_closed && _next && ((_next->hasChanged() && _current && (_current->getValue() != _next->getValue())) || ! _current));
 }
 
 int64_t
@@ -88,7 +95,7 @@ ConfigSubscription::flip()
 {
     bool change = hasChanged();
     if (change) {
-        _current.reset(_next.release());
+        _current = std::move(_next);
         _lastGenerationChanged = _current->getGeneration();
     } else {
         _current.reset(new ConfigUpdate(_current->getValue(), false, _next->getGeneration()));
@@ -96,13 +103,15 @@ ConfigSubscription::flip()
     _isChanged = change;
 }
 
-ConfigValue
+const ConfigValue &
 ConfigSubscription::getConfig() const
 {
-    if (_closed)
+    if (_closed) {
         throw ConfigRuntimeException("Subscription is closed, config no longer available");
-    if (_current.get() == NULL)
+    }
+    if ( ! _current) {
         throw ConfigRuntimeException("No configuration available");
+    }
     return _current->getValue();
 }
 
diff --git a/config/src/vespa/config/subscription/configsubscription.h b/config/src/vespa/config/subscription/configsubscription.h
index 04c3da114fd..56b65922a61 100644
--- a/config/src/vespa/config/subscription/configsubscription.h
+++ b/config/src/vespa/config/subscription/configsubscription.h
@@ -28,7 +28,7 @@ public:
      *
      * @return the current ConfigValue.
      */
-    ConfigValue getConfig() const;
+    const ConfigValue & getConfig() const;
 
     /**
      * Checks whether or not the config has changed.
@@ -48,6 +48,7 @@ public:
     bool nextUpdate(int64_t generation, uint64_t timeoutInMillis);
     int64_t getGeneration() const;
     bool hasChanged() const;
+    bool hasGenerationChanged() const;
     void flip();
     void reset();
     void close();
diff --git a/config/src/vespa/config/subscription/configsubscriptionset.cpp b/config/src/vespa/config/subscription/configsubscriptionset.cpp
index f6268c8a84a..ab38ba18351 100644
--- a/config/src/vespa/config/subscription/configsubscriptionset.cpp
+++ b/config/src/vespa/config/subscription/configsubscriptionset.cpp
@@ -38,10 +38,6 @@ ConfigSubscriptionSet::acquireSnapshot(uint64_t timeoutInMillis, bool ignoreChan
     int64_t lastGeneration = _currentGeneration;
     bool inSync = false;
 
-    for (const auto & subscription : _subscriptionList) {
-        subscription->reset();
-    }
-
     LOG(debug, "Going into nextConfig loop, time left is %d", timeLeft);
     while (_state != CLOSED && timeLeft >= 0 && !inSync) {
         size_t numChanged = 0;
@@ -52,8 +48,10 @@ ConfigSubscriptionSet::acquireSnapshot(uint64_t timeoutInMillis, bool ignoreChan
         // Run nextUpdate on all subscribers to get them in sync.
         for (const auto & subscription : _subscriptionList) {
 
-            if (!subscription->nextUpdate(_currentGeneration, timeLeft))
-                break;
+            if (!subscription->nextUpdate(_currentGeneration, timeLeft) && !subscription->hasGenerationChanged()) {
+                subscription->reset();
+                continue;
+            }
 
             const ConfigKey & key(subscription->getKey());
             if (subscription->hasChanged()) {