Avoid race when getting config

* Always add to delayed responses (we remove instead if we find config in cache)
  This is to avoid a race where we might end up not adding to delayed responses
  nor subscribing to config if another request for the same config
  happens at the same time
* Add to cache only in one place
* Remove meaningless test

8 files changed, 42 insertions, 51 deletions

diff --git a/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/ClientUpdater.java b/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/ClientUpdater.java
index 28fc22538b6..f4ae5e130e1 100644
--- a/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/ClientUpdater.java
+++ b/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/ClientUpdater.java
@@ -104,4 +104,9 @@ class ClientUpdater {
             log.log(LogLevel.DEBUG, "Finished updating config for " + config.getKey() + "," + config.getGeneration());
         }
     }
+
+    // TODO: Remove, temporary until MapBackedConfigSource has been refactored
+    public MemoryCache getMemoryCache() {
+        return memoryCache;
+    }
 }
diff --git a/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/MapBackedConfigSource.java b/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/MapBackedConfigSource.java
index e8a76257b14..57adaa572bd 100644
--- a/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/MapBackedConfigSource.java
+++ b/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/MapBackedConfigSource.java
@@ -17,6 +17,7 @@ import java.util.List;
  * @author hmusum
  * @since 5.1.10
  */
+// TODO Move to src/test/
 public class MapBackedConfigSource implements ConfigSource, ConfigSourceClient {
     private final HashMap<ConfigKey<?>, RawConfig> backing = new HashMap<>();
     private final ClientUpdater clientUpdater;
@@ -33,7 +34,9 @@ public class MapBackedConfigSource implements ConfigSource, ConfigSourceClient {
 
     @Override
     public RawConfig getConfig(RawConfig input, JRTServerConfigRequest request) {
-        return getConfig(input.getKey());
+        final RawConfig config = getConfig(input.getKey());
+        clientUpdater.getMemoryCache().put(config);
+        return config;
     }
 
     RawConfig getConfig(ConfigKey<?> configKey) {
diff --git a/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/ProxyServer.java b/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/ProxyServer.java
index c5df12f1e96..463fa163b20 100644
--- a/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/ProxyServer.java
+++ b/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/ProxyServer.java
@@ -85,19 +85,21 @@ public class ProxyServer implements Runnable {
     }
 
     static ProxyServer createTestServer(ConfigSourceSet source) {
-        return createTestServer(source, null);
+        return createTestServer(source, null, new MemoryCache());
     }
 
-    static ProxyServer createTestServer(MapBackedConfigSource source) {
-        return createTestServer(source, source);
+    static ProxyServer createTestServer(MapBackedConfigSource source, MemoryCache memoryCache) {
+        return createTestServer(source, source, memoryCache);
     }
 
-    private static ProxyServer createTestServer(ConfigSource source, ConfigSourceClient configSourceClient) {
+    private static ProxyServer createTestServer(ConfigSource source,
+                                                ConfigSourceClient configSourceClient,
+                                                MemoryCache memoryCache) {
         final ConfigProxyStatistics statistics = new ConfigProxyStatistics();
         final boolean delayedResponseHandling = false;
         return new ProxyServer(null, new DelayedResponses(statistics),
                                source, statistics, defaultTimingValues(), delayedResponseHandling,
-                               new MemoryCache(), configSourceClient);
+                               memoryCache, configSourceClient);
     }
 
     public void run() {
@@ -123,11 +125,7 @@ public class ProxyServer implements Runnable {
         // create a background thread that retrieves config from the server and
         // calls updateSubscribers when new config is returned from the config source.
         // In the last case the method below will return null.
-        RawConfig config = configClient.getConfig(RawConfig.createFromServerRequest(req), req);
-        if (configOrGenerationHasChanged(config, req)) {
-            memoryCache.put(config);
-        }
-        return config;
+        return configClient.getConfig(RawConfig.createFromServerRequest(req), req);
     }
 
     static boolean configOrGenerationHasChanged(RawConfig config, JRTServerConfigRequest request) {
diff --git a/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/RpcConfigSourceClient.java b/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/RpcConfigSourceClient.java
index b0ad57e8a6c..a2df139ad5d 100644
--- a/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/RpcConfigSourceClient.java
+++ b/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/RpcConfigSourceClient.java
@@ -112,34 +112,37 @@ class RpcConfigSourceClient implements ConfigSourceClient {
      */
     @Override
     public RawConfig getConfig(RawConfig input, JRTServerConfigRequest request) {
-        long start = System.currentTimeMillis();
-        RawConfig ret = null;
+        // Always add to delayed responses (we remove instead if we find config in cache)
+        // This is to avoid a race where we might end up not adding to delayed responses
+        // nor subscribing to config if another request for the same config
+        // happens at the same time
+        DelayedResponse delayedResponse = new DelayedResponse(request);
+        delayedResponses.add(delayedResponse);
+
         final ConfigCacheKey configCacheKey = new ConfigCacheKey(input.getKey(), input.getDefMd5());
         RawConfig cachedConfig = memoryCache.get(configCacheKey);
         boolean needToGetConfig = true;
 
+        RawConfig ret = null;
         if (cachedConfig != null) {
             log.log(LogLevel.DEBUG, "Found config " + configCacheKey + " in cache, generation=" + cachedConfig.getGeneration() +
                     ",configmd5=" + cachedConfig.getConfigMd5());
             if (log.isLoggable(LogLevel.SPAM)) {
                 log.log(LogLevel.SPAM, "input config=" + input + ",cached config=" + cachedConfig);
             }
-            // equals() also takes generation into account
-            if (!cachedConfig.equals(input)) {
+            if (ProxyServer.configOrGenerationHasChanged(cachedConfig, request)) {
                 log.log(LogLevel.SPAM, "Cached config is not equal to requested, will return it");
                 ret = cachedConfig;
+
+                // Someone else has replied
+                if (! delayedResponses.remove(delayedResponse)) {
+                    ret = null;
+                }
             }
             if (!cachedConfig.isError()) {
                 needToGetConfig = false;
             }
         }
-        if (!ProxyServer.configOrGenerationHasChanged(ret, request)) {
-            if (log.isLoggable(LogLevel.DEBUG)) {
-                log.log(LogLevel.DEBUG, "Delaying response " + request.getShortDescription() + " (" +
-                        (System.currentTimeMillis() - start) + " ms)");
-            }
-            delayedResponses.add(new DelayedResponse(request));
-        }
         if (needToGetConfig) {
             subscribeToConfig(input, configCacheKey);
         }
diff --git a/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/UpstreamConfigSubscriber.java b/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/UpstreamConfigSubscriber.java
index 7a8a4436749..85e60e28b70 100644
--- a/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/UpstreamConfigSubscriber.java
+++ b/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/UpstreamConfigSubscriber.java
@@ -68,6 +68,7 @@ public class UpstreamConfigSubscriber implements Subscriber {
                     "', generation=" + newConfig.getGeneration() +
                     ", payload=" + newConfig.getPayload());
         }
+        // memoryCache.put(); TODO: Add here later and remove in ClientUpdater
         clientUpdater.updateSubscribers(newConfig);
     }
 
diff --git a/config-proxy/src/test/java/com/yahoo/vespa/config/proxy/DelayedResponseHandlerTest.java b/config-proxy/src/test/java/com/yahoo/vespa/config/proxy/DelayedResponseHandlerTest.java
index f063427d808..7ba4d676000 100644
--- a/config-proxy/src/test/java/com/yahoo/vespa/config/proxy/DelayedResponseHandlerTest.java
+++ b/config-proxy/src/test/java/com/yahoo/vespa/config/proxy/DelayedResponseHandlerTest.java
@@ -14,7 +14,7 @@ import static org.junit.Assert.assertThat;
  */
 public class DelayedResponseHandlerTest {
 
-    private final MapBackedConfigSource source = new MapBackedConfigSource(UpstreamConfigSubscriberTest.MockClientUpdater.create());
+    private final MapBackedConfigSource source = new MapBackedConfigSource(UpstreamConfigSubscriberTest.MockClientUpdater.create(new MemoryCache()));
 
     @Rule
     public TemporaryFolder temporaryFolder = new TemporaryFolder();
diff --git a/config-proxy/src/test/java/com/yahoo/vespa/config/proxy/ProxyServerTest.java b/config-proxy/src/test/java/com/yahoo/vespa/config/proxy/ProxyServerTest.java
index ed6d2056733..47b4dd4d1ea 100644
--- a/config-proxy/src/test/java/com/yahoo/vespa/config/proxy/ProxyServerTest.java
+++ b/config-proxy/src/test/java/com/yahoo/vespa/config/proxy/ProxyServerTest.java
@@ -21,8 +21,9 @@ import static org.junit.Assert.*;
  */
 public class ProxyServerTest {
 
-    private final MapBackedConfigSource source = new MapBackedConfigSource(UpstreamConfigSubscriberTest.MockClientUpdater.create());
-    private ProxyServer proxy = ProxyServer.createTestServer(source);
+    MemoryCache memoryCache = new MemoryCache();
+    private final MapBackedConfigSource source = new MapBackedConfigSource(UpstreamConfigSubscriberTest.MockClientUpdater.create(memoryCache));
+    private ProxyServer proxy = ProxyServer.createTestServer(source, memoryCache);
 
     static final RawConfig fooConfig = Helper.fooConfigV2;
 
@@ -40,7 +41,7 @@ public class ProxyServerTest {
         source.clear();
         source.put(fooConfig.getKey(), createConfigWithNextConfigGeneration(fooConfig, 0));
         source.put(errorConfigKey, createConfigWithNextConfigGeneration(fooConfig, ErrorCode.UNKNOWN_DEFINITION));
-        proxy = ProxyServer.createTestServer(source);
+        proxy = ProxyServer.createTestServer(source, memoryCache);
     }
 
     @After
@@ -81,26 +82,6 @@ public class ProxyServerTest {
     }
 
     /**
-     * Tests that the proxy server can be tested with a MapBackedConfigSource,
-     * which is a simple hash map with configs
-     */
-    @Test
-    public void testRawConfigSetBasics() {
-        ConfigTester tester = new ConfigTester();
-        JRTServerConfigRequest errorConfigRequest = tester.createRequest(errorConfig);
-
-        assertTrue(proxy.getMode().isDefault());
-        RawConfig config = proxy.resolveConfig(Helper.fooConfigRequest);
-        assertThat(config, is(createConfigWithNextConfigGeneration(Helper.fooConfig, 0)));
-
-        config = proxy.resolveConfig(Helper.barConfigRequest);
-        assertNull(config);
-
-        config = proxy.resolveConfig(errorConfigRequest);
-        assertThat(config.errorCode(), is(ErrorCode.UNKNOWN_DEFINITION));
-    }
-
-    /**
      * Verifies that config is retrieved from the real server when it is not found in the cache,
      * that the cache is populated with the config and that the entry in the cache is used
      * when it is found there.
diff --git a/config-proxy/src/test/java/com/yahoo/vespa/config/proxy/UpstreamConfigSubscriberTest.java b/config-proxy/src/test/java/com/yahoo/vespa/config/proxy/UpstreamConfigSubscriberTest.java
index 27ee550d956..e566df8bb2a 100644
--- a/config-proxy/src/test/java/com/yahoo/vespa/config/proxy/UpstreamConfigSubscriberTest.java
+++ b/config-proxy/src/test/java/com/yahoo/vespa/config/proxy/UpstreamConfigSubscriberTest.java
@@ -43,7 +43,7 @@ public class UpstreamConfigSubscriberTest {
 
     @Before
     public void setup() {
-        clientUpdater = MockClientUpdater.create();
+        clientUpdater = MockClientUpdater.create(new MemoryCache());
         sourceResponses = new MapBackedConfigSource(clientUpdater);
 
         ConfigPayload payload = getConfigPayload("bar", "value");
@@ -147,14 +147,14 @@ public class UpstreamConfigSubscriberTest {
     static class MockClientUpdater extends ClientUpdater {
         private RawConfig lastConfig;
 
-        private MockClientUpdater(ConfigProxyStatistics statistics, Mode mode) {
-            super(new MemoryCache(), new MockRpcServer(), statistics, new DelayedResponses(statistics), mode);
+        private MockClientUpdater(ConfigProxyStatistics statistics, Mode mode, MemoryCache memoryCache) {
+            super(memoryCache, new MockRpcServer(), statistics, new DelayedResponses(statistics), mode);
         }
 
-        public static MockClientUpdater create() {
+        static MockClientUpdater create(MemoryCache memoryCache) {
             Mode mode = new Mode();
             ConfigProxyStatistics statistics = new ConfigProxyStatistics();
-            return new MockClientUpdater(statistics, mode);
+            return new MockClientUpdater(statistics, mode, memoryCache);
         }
 
         @Override