From 962805f9497201f1c254a41a3297727a563ecf9d Mon Sep 17 00:00:00 2001 From: Kris De Volder Date: Fri, 21 Jul 2017 17:13:41 -0700 Subject: [PATCH] Fix issue with CachinhModelProvider not caching 'failed' results Also tweak some timeout values to try to make dynamic CA and reconcile more responsive. --- .../.settings/org.eclipse.jdt.ui.prefs | 2 +- .../models/BoshCommandBasedModelProvider.java | 15 +++-- .../bosh/models/CachingModelProvider.java | 21 +++++-- .../bosh/mocks/MockCloudConfigProvider.java | 2 +- .../BoshCommandCloudConfigProviderTest.java | 2 +- .../BoshCommandStemcellsProviderTest.java | 1 - .../bosh/models/CachingModelProviderTest.java | 56 +++++++++++++++++++ 7 files changed, 86 insertions(+), 13 deletions(-) rename headless-services/bosh-language-server/src/test/java/org/springframework/ide/vscode/bosh/{ => models}/BoshCommandCloudConfigProviderTest.java (97%) create mode 100644 headless-services/bosh-language-server/src/test/java/org/springframework/ide/vscode/bosh/models/CachingModelProviderTest.java diff --git a/headless-services/bosh-language-server/.settings/org.eclipse.jdt.ui.prefs b/headless-services/bosh-language-server/.settings/org.eclipse.jdt.ui.prefs index 75f829439..68f26e829 100644 --- a/headless-services/bosh-language-server/.settings/org.eclipse.jdt.ui.prefs +++ b/headless-services/bosh-language-server/.settings/org.eclipse.jdt.ui.prefs @@ -27,7 +27,7 @@ sp_cleanup.make_variable_declarations_final=false sp_cleanup.never_use_blocks=false sp_cleanup.never_use_parentheses_in_expressions=true sp_cleanup.on_save_use_additional_actions=true -sp_cleanup.organize_imports=true +sp_cleanup.organize_imports=false sp_cleanup.qualify_static_field_accesses_with_declaring_class=false sp_cleanup.qualify_static_member_accesses_through_instances_with_declaring_class=true sp_cleanup.qualify_static_member_accesses_through_subtypes_with_declaring_class=true diff --git a/headless-services/bosh-language-server/src/main/java/org/springframework/ide/vscode/bosh/models/BoshCommandBasedModelProvider.java b/headless-services/bosh-language-server/src/main/java/org/springframework/ide/vscode/bosh/models/BoshCommandBasedModelProvider.java index 4b8c899e6..d6227aaa9 100644 --- a/headless-services/bosh-language-server/src/main/java/org/springframework/ide/vscode/bosh/models/BoshCommandBasedModelProvider.java +++ b/headless-services/bosh-language-server/src/main/java/org/springframework/ide/vscode/bosh/models/BoshCommandBasedModelProvider.java @@ -37,7 +37,7 @@ public abstract class BoshCommandBasedModelProvider implements DynamicModelPr private final YamlParser yamlParser; protected final ObjectMapper mapper = new ObjectMapper().configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false); - protected Duration CMD_TIMEOUT = Duration.ofSeconds(10); + protected Duration CMD_TIMEOUT = Duration.ofSeconds(3); protected BoshCommandBasedModelProvider() { Representer representer = new Representer(); @@ -76,10 +76,15 @@ public abstract class BoshCommandBasedModelProvider implements DynamicModelPr protected String executeCommand(ExternalCommand command) throws Exception { Log.info("executing cmd: "+command); - ExternalProcess process = new ExternalProcess(getWorkingDir(), command, true, CMD_TIMEOUT); - Log.info("executing cmd DONE: "+process); - String out = process.getOut(); - return out; + try { + ExternalProcess process = new ExternalProcess(getWorkingDir(), command, true, CMD_TIMEOUT); + Log.info("executing cmd SUCCESS: "+process); + String out = process.getOut(); + return out; + } catch (Exception e) { + Log.log("executing cmd FAILED", e); + throw e; + } } protected File getWorkingDir() { diff --git a/headless-services/bosh-language-server/src/main/java/org/springframework/ide/vscode/bosh/models/CachingModelProvider.java b/headless-services/bosh-language-server/src/main/java/org/springframework/ide/vscode/bosh/models/CachingModelProvider.java index c843daed7..964d93814 100644 --- a/headless-services/bosh-language-server/src/main/java/org/springframework/ide/vscode/bosh/models/CachingModelProvider.java +++ b/headless-services/bosh-language-server/src/main/java/org/springframework/ide/vscode/bosh/models/CachingModelProvider.java @@ -10,6 +10,7 @@ *******************************************************************************/ package org.springframework.ide.vscode.bosh.models; +import java.util.concurrent.CompletableFuture; import java.util.concurrent.TimeUnit; import java.util.function.Function; @@ -29,9 +30,9 @@ public class CachingModelProvider implements DynamicModelProvider { */ private static final Object NULL_KEY = new Object(); - private long timeout = 15; + private long timeout = 30; private TimeUnit timeoutUnit = TimeUnit.SECONDS; - private Cache cache = createCache(); + private Cache> cache = createCache(); private final DynamicModelProvider delegate; @@ -48,7 +49,7 @@ public class CachingModelProvider implements DynamicModelProvider { */ private Function keyGetter = (dc) -> "WHATEVER"; - protected Cache createCache() { + protected Cache> createCache() { return CacheBuilder.newBuilder() .expireAfterWrite(timeout, timeoutUnit) .build(); @@ -67,7 +68,19 @@ public class CachingModelProvider implements DynamicModelProvider { //guava cache doesn't like null key key = NULL_KEY; } - return cache.get(key, () -> delegate.getModel(dc)); + CompletableFuture cached; + synchronized (this) { + cached = cache.get(key, () -> { + try { + return CompletableFuture.completedFuture(delegate.getModel(dc)); + } catch (Throwable e) { + CompletableFuture failed = new CompletableFuture<>(); + failed.completeExceptionally(e); + return failed; + } + }); + } + return cached.get(); } } diff --git a/headless-services/bosh-language-server/src/test/java/org/springframework/ide/vscode/bosh/mocks/MockCloudConfigProvider.java b/headless-services/bosh-language-server/src/test/java/org/springframework/ide/vscode/bosh/mocks/MockCloudConfigProvider.java index 02e96ea39..e70e76fd2 100644 --- a/headless-services/bosh-language-server/src/test/java/org/springframework/ide/vscode/bosh/mocks/MockCloudConfigProvider.java +++ b/headless-services/bosh-language-server/src/test/java/org/springframework/ide/vscode/bosh/mocks/MockCloudConfigProvider.java @@ -12,8 +12,8 @@ package org.springframework.ide.vscode.bosh.mocks; import java.util.concurrent.Callable; -import org.springframework.ide.vscode.bosh.BoshCommandCloudConfigProviderTest; import org.springframework.ide.vscode.bosh.models.BoshCommandCloudConfigProvider; +import org.springframework.ide.vscode.bosh.models.BoshCommandCloudConfigProviderTest; import org.springframework.ide.vscode.commons.util.ExternalCommand; import org.springframework.ide.vscode.commons.util.IOUtil; diff --git a/headless-services/bosh-language-server/src/test/java/org/springframework/ide/vscode/bosh/BoshCommandCloudConfigProviderTest.java b/headless-services/bosh-language-server/src/test/java/org/springframework/ide/vscode/bosh/models/BoshCommandCloudConfigProviderTest.java similarity index 97% rename from headless-services/bosh-language-server/src/test/java/org/springframework/ide/vscode/bosh/BoshCommandCloudConfigProviderTest.java rename to headless-services/bosh-language-server/src/test/java/org/springframework/ide/vscode/bosh/models/BoshCommandCloudConfigProviderTest.java index a0a551059..04c14df5f 100644 --- a/headless-services/bosh-language-server/src/test/java/org/springframework/ide/vscode/bosh/BoshCommandCloudConfigProviderTest.java +++ b/headless-services/bosh-language-server/src/test/java/org/springframework/ide/vscode/bosh/models/BoshCommandCloudConfigProviderTest.java @@ -8,7 +8,7 @@ * Contributors: * Pivotal, Inc. - initial API and implementation *******************************************************************************/ -package org.springframework.ide.vscode.bosh; +package org.springframework.ide.vscode.bosh.models; import static org.junit.Assert.assertEquals; diff --git a/headless-services/bosh-language-server/src/test/java/org/springframework/ide/vscode/bosh/models/BoshCommandStemcellsProviderTest.java b/headless-services/bosh-language-server/src/test/java/org/springframework/ide/vscode/bosh/models/BoshCommandStemcellsProviderTest.java index 3288cd572..5d0b48a17 100644 --- a/headless-services/bosh-language-server/src/test/java/org/springframework/ide/vscode/bosh/models/BoshCommandStemcellsProviderTest.java +++ b/headless-services/bosh-language-server/src/test/java/org/springframework/ide/vscode/bosh/models/BoshCommandStemcellsProviderTest.java @@ -15,7 +15,6 @@ import static org.junit.Assert.assertEquals; import org.junit.Before; import org.junit.Test; import org.mockito.Mockito; -import org.springframework.ide.vscode.bosh.BoshCommandCloudConfigProviderTest; import org.springframework.ide.vscode.commons.util.IOUtil; import org.springframework.ide.vscode.commons.yaml.schema.DynamicSchemaContext; diff --git a/headless-services/bosh-language-server/src/test/java/org/springframework/ide/vscode/bosh/models/CachingModelProviderTest.java b/headless-services/bosh-language-server/src/test/java/org/springframework/ide/vscode/bosh/models/CachingModelProviderTest.java new file mode 100644 index 000000000..86d7ff2da --- /dev/null +++ b/headless-services/bosh-language-server/src/test/java/org/springframework/ide/vscode/bosh/models/CachingModelProviderTest.java @@ -0,0 +1,56 @@ +/******************************************************************************* + * Copyright (c) 2017 Pivotal, Inc. + * All rights reserved. This program and the accompanying materials + * are made available under the terms of the Eclipse Public License v1.0 + * which accompanies this distribution, and is available at + * http://www.eclipse.org/legal/epl-v10.html + * + * Contributors: + * Pivotal, Inc. - initial API and implementation + *******************************************************************************/ +package org.springframework.ide.vscode.bosh.models; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.fail; +import static org.mockito.Mockito.*; + +import java.util.concurrent.ExecutionException; +import java.util.concurrent.TimeoutException; + +import org.junit.Test; +import org.springframework.ide.vscode.commons.util.ExceptionUtil; + +@SuppressWarnings("unchecked") +public class CachingModelProviderTest { + + @Test public void goodValuesAreCached() throws Exception { + DynamicModelProvider modelProvider = mock(DynamicModelProvider.class); + when(modelProvider.getModel(any())).thenReturn("RESULT"); + + DynamicModelProvider cached = new CachingModelProvider<>(modelProvider); + + assertEquals("RESULT", cached.getModel(null)); + assertEquals("RESULT", cached.getModel(null)); + assertEquals("RESULT", cached.getModel(null)); + + verify(modelProvider, times(1)).getModel(any()); + } + + @Test public void timeoutExceptionsAreCached() throws Exception { + DynamicModelProvider modelProvider = mock(DynamicModelProvider.class); + when(modelProvider.getModel(any())).thenThrow(new TimeoutException("timed out")); + + DynamicModelProvider cached = new CachingModelProvider<>(modelProvider); + for (int i = 0; i < 3; i++) { + try { + cached.getModel(null); + fail("Should have thrown"); + } catch (Exception _e) { + Throwable e = ExceptionUtil.getDeepestCause(_e); + assertEquals(TimeoutException.class, e.getClass()); + } + } + verify(modelProvider, times(1)).getModel(any()); + } + +}