diff --git a/tds/src/integrationTests/java/thredds/server/wms/TestUpdateWmsServer.java b/tds/src/integrationTests/java/thredds/server/wms/TestUpdateWmsServer.java index bb11f8cdba..d3b477c235 100644 --- a/tds/src/integrationTests/java/thredds/server/wms/TestUpdateWmsServer.java +++ b/tds/src/integrationTests/java/thredds/server/wms/TestUpdateWmsServer.java @@ -5,6 +5,7 @@ package thredds.server.wms; +import java.io.File; import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; @@ -19,9 +20,9 @@ import org.jdom2.input.SAXBuilder; import org.jdom2.xpath.XPathExpression; import org.jdom2.xpath.XPathFactory; -import org.junit.After; -import org.junit.Ignore; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.TemporaryFolder; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import java.io.IOException; @@ -33,36 +34,40 @@ import static com.google.common.truth.Truth.assertThat; -@Ignore("TODO: fix locked files issue") public class TestUpdateWmsServer { private static final Logger logger = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass()); private static final Namespace NS_WMS = Namespace.getNamespace("wms", "http://www.opengis.net/wms"); private static final String DIR = "src/test/content/thredds/public/testdata/"; - private static final Path TEST_FILE = Paths.get(DIR, "testUpdate.nc"); + private static final Path SRC_FILE_1 = Paths.get(DIR, "testGridAsPoint.nc"); + private static final Path SRC_FILE_2 = Paths.get(DIR, "testData.nc"); + private static final String FILENAME = "testUpdate.nc"; - @After - public void cleanupTestFile() throws IOException { - Files.delete(TEST_FILE); - } + @Rule + public TemporaryFolder temporaryFolder = new TemporaryFolder(new File(DIR)); @Test public void testUpdateFile() throws IOException, JDOMException { - final String path = "/wms/localContent/testUpdate.nc?service=WMS&version=1.3.0&request=GetCapabilities"; + final String filePath = temporaryFolder.getRoot().getName() + "/" + FILENAME; + final String path = "/wms/localContent/" + filePath + "?service=WMS&version=1.3.0&request=GetCapabilities"; final String endpoint = TestOnLocalServer.withHttpPath(path); // Check initial WMS output - Files.copy(Paths.get(DIR, "testGridAsPoint.nc"), TEST_FILE); + createTestFile(SRC_FILE_1, FILENAME); final byte[] result = TestOnLocalServer.getContent(endpoint, HttpServletResponse.SC_OK, ContentType.xmlwms); checkLayerNameInXml(result, "withT1Z1"); // Update test file and check that WMS output is updated - Files.copy(Paths.get(DIR, "testData.nc"), TEST_FILE, StandardCopyOption.REPLACE_EXISTING); + createTestFile(SRC_FILE_2, FILENAME); final byte[] updatedResult = TestOnLocalServer.getContent(endpoint, HttpServletResponse.SC_OK, ContentType.xmlwms); checkLayerNameInXml(updatedResult, "Z_sfc"); } + private void createTestFile(Path srcFile, String filename) throws IOException { + Files.copy(srcFile, Paths.get(temporaryFolder.getRoot().toString(), filename), StandardCopyOption.REPLACE_EXISTING); + } + private void checkLayerNameInXml(byte[] result, String expectedLayerName) throws IOException, JDOMException { final Reader reader = new StringReader(new String(result, StandardCharsets.UTF_8)); final SAXBuilder saxBuilder = new SAXBuilder(); diff --git a/tds/src/main/java/thredds/server/admin/DebugCommands.java b/tds/src/main/java/thredds/server/admin/DebugCommands.java index b6c5f55c40..c22c8761e6 100644 --- a/tds/src/main/java/thredds/server/admin/DebugCommands.java +++ b/tds/src/main/java/thredds/server/admin/DebugCommands.java @@ -10,6 +10,7 @@ import org.springframework.stereotype.Component; import thredds.featurecollection.cache.GridInventoryCacheChronicle; import thredds.server.config.TdsContext; +import thredds.server.wms.ThreddsWmsServlet; import thredds.servlet.ServletUtil; import ucar.nc2.dataset.NetcdfDataset; import java.io.ByteArrayOutputStream; @@ -149,6 +150,9 @@ public void doAction(Event e) { f.format("%n%n"); GridInventoryCacheChronicle.showCache(f); + f.format("%n%n"); + ThreddsWmsServlet.showCache(f); + e.pw.flush(); } }; @@ -162,6 +166,7 @@ public void doAction(Event e) { FileCacheIF fc = GribCdmIndex.gribCollectionCache; if (fc != null) fc.clearCache(false); + ThreddsWmsServlet.resetCache(); e.pw.println(" ClearCache ok"); } }; @@ -220,6 +225,13 @@ public void doAction(Event e) { }; debugHandler.addAction(act); + act = new Action("forceWMSCache", "Force clear WMS Cache") { + public void doAction(Event e) { + ThreddsWmsServlet.resetCache(); + e.pw.println(" WMS force clearCache done"); + } + }; + debugHandler.addAction(act); } protected void makeDebugActions() { diff --git a/tds/src/main/java/thredds/server/wms/TdsWmsDatasetFactory.java b/tds/src/main/java/thredds/server/wms/TdsWmsDatasetFactory.java index f0933e9452..4b212f40ca 100644 --- a/tds/src/main/java/thredds/server/wms/TdsWmsDatasetFactory.java +++ b/tds/src/main/java/thredds/server/wms/TdsWmsDatasetFactory.java @@ -22,15 +22,6 @@ public void setNetcdfDataset(NetcdfDataset ncd) { this.netcdfDataset = ncd; } - /** - * Get time of last modification of the underlying netcdfDataset - * - * @return time of last modification in Unix time (msecs since reference), or 0 if unknown - */ - long getLastModified() { - return netcdfDataset.getLastModified(); - } - /** * * None of this matters really. For NcML datasets, we cannot rely on a location, as the diff --git a/tds/src/main/java/thredds/server/wms/ThreddsWmsCatalogue.java b/tds/src/main/java/thredds/server/wms/ThreddsWmsCatalogue.java index 5a3ccc41f2..752a16c388 100644 --- a/tds/src/main/java/thredds/server/wms/ThreddsWmsCatalogue.java +++ b/tds/src/main/java/thredds/server/wms/ThreddsWmsCatalogue.java @@ -147,15 +147,6 @@ public String getTdsDatasetPath() { return tdsDatasetPath; } - /** - * Get time of last modification of the underlying netcdfDataset - * - * @return time of last modification in Unix time (msecs since reference), or 0 if unknown - */ - long getLastModified() { - return datasetFactory.getLastModified(); - } - @Override public FeaturesAndMemberName getFeaturesForLayer(String layerName, PlottingDomainParams params) throws EdalException { /* @@ -431,4 +422,8 @@ public EnhancedVariableMetadata getLayerMetadata(final VariableMetadata metadata */ return new TdsEnhancedVariableMetadata(this, metadata); } + + void setNetcdfDataset(NetcdfDataset ncd) { + datasetFactory.setNetcdfDataset(ncd); + } } diff --git a/tds/src/main/java/thredds/server/wms/ThreddsWmsServlet.java b/tds/src/main/java/thredds/server/wms/ThreddsWmsServlet.java index 3eaaa0d0a9..fb8285c68c 100644 --- a/tds/src/main/java/thredds/server/wms/ThreddsWmsServlet.java +++ b/tds/src/main/java/thredds/server/wms/ThreddsWmsServlet.java @@ -28,6 +28,10 @@ package thredds.server.wms; +import com.google.common.cache.Cache; +import com.google.common.cache.CacheBuilder; +import java.io.IOException; +import java.util.Formatter; import org.springframework.stereotype.Controller; import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.RequestMethod; @@ -36,7 +40,6 @@ import uk.ac.rdg.resc.edal.wms.RequestParams; import uk.ac.rdg.resc.edal.wms.WmsCatalogue; import uk.ac.rdg.resc.edal.wms.WmsServlet; -import java.util.HashMap; import java.util.Map; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; @@ -70,16 +73,14 @@ public CachedWmsCatalogue(ThreddsWmsCatalogue wmsCatalogue, long lastModified) { } } - private static final Map catalogueCache = new HashMap<>(); - - static void resetCache() { - catalogueCache.clear(); - } + private static final Cache catalogueCache = + CacheBuilder.newBuilder().maximumSize(200).recordStats().build(); + private static int cacheLoads = 0; @Override @RequestMapping(value = "**", method = {RequestMethod.GET}) protected void dispatchWmsRequest(String request, RequestParams params, HttpServletRequest httpServletRequest, - HttpServletResponse httpServletResponse, WmsCatalogue catalogue) throws Exception { + HttpServletResponse httpServletResponse, WmsCatalogue wmsCatalogue) throws Exception { /* * The super implementation of this gets called with a servlet-wide * catalogue, which "should" have been injected with the @@ -95,60 +96,75 @@ protected void dispatchWmsRequest(String request, RequestParams params, HttpServ // Look - is setting this to null the right thing to do?? String removePrefix = null; TdsRequestedDataset tdsDataset = new TdsRequestedDataset(httpServletRequest, removePrefix); - if (useCachedCatalogue(tdsDataset.getPath())) { - catalogue = catalogueCache.get(tdsDataset.getPath()).wmsCatalogue; - } else { - NetcdfFile ncf = TdsRequestedDataset.getNetcdfFile(httpServletRequest, httpServletResponse, tdsDataset.getPath()); - NetcdfDataset ncd; - if (TdsRequestedDataset.useNetcdfJavaBuilders()) { - ncd = NetcdfDatasets.enhance(ncf, NetcdfDataset.getDefaultEnhanceMode(), null); - } else { - ncd = NetcdfDataset.wrap(ncf, NetcdfDataset.getDefaultEnhanceMode()); - } - - String netcdfFilePath = ncf.getLocation(); + try (NetcdfDataset ncd = acquireNetcdfDataset(httpServletRequest, httpServletResponse, tdsDataset.getPath())) { + ThreddsWmsCatalogue catalogue = acquireCatalogue(ncd, tdsDataset.getPath()); /* - * Generate a new catalogue for the given dataset - * - * In the full system, we should keep a cache of these - * ThreddsWmsCatalogues, but in this example we just create each new one - * on the fly. - * - * If a feature cache is required on the WMS (a Good Idea), I recommend - * a single cache in this servlet which gets passed to each WmsCatalogue - * upon construction (i.e. HERE). That's a TDS implementation detail - * though, hence not in this example. + * Now that we've got a WmsCatalogue, we can pass this request to the + * super implementation which will handle things from here. */ - if (netcdfFilePath == null) { - throw new EdalLayerNotFoundException("The requested dataset is not available on this server"); - } - catalogue = new ThreddsWmsCatalogue(ncd, tdsDataset.getPath()); - final CachedWmsCatalogue cachedWmsCatalogue = - new CachedWmsCatalogue((ThreddsWmsCatalogue) catalogue, ncd.getLastModified()); - catalogueCache.put(tdsDataset.getPath(), cachedWmsCatalogue); + super.dispatchWmsRequest(request, params, httpServletRequest, httpServletResponse, catalogue); } + } - /* - * Now that we've got a WmsCatalogue, we can pass this request to the - * super implementation which will handle things from here. - */ - super.dispatchWmsRequest(request, params, httpServletRequest, httpServletResponse, catalogue); + private ThreddsWmsCatalogue acquireCatalogue(NetcdfDataset ncd, String tdsDatasetPath) throws IOException { + if (ncd.getLocation() == null) { + throw new EdalLayerNotFoundException("The requested dataset is not available on this server"); + } + + final CachedWmsCatalogue cachedWmsCatalogue = catalogueCache.getIfPresent(tdsDatasetPath); + final long lastModified = ncd.getLastModified(); + + if (cachedWmsCatalogue != null && cachedWmsCatalogue.lastModified >= lastModified) { + // Must update NetcdfDataset to ensure file resources are reacquired, as this has been closed. + // But we don't need to recreate the ThreddsWmsCatalogue as it is up-to-date according to the last modified + cachedWmsCatalogue.wmsCatalogue.setNetcdfDataset(ncd); + return cachedWmsCatalogue.wmsCatalogue; + } else { + // Create and put/ replace in cache + ThreddsWmsCatalogue threddsWmsCatalogue = new ThreddsWmsCatalogue(ncd, tdsDatasetPath); + catalogueCache.put(tdsDatasetPath, new CachedWmsCatalogue(threddsWmsCatalogue, lastModified)); + cacheLoads++; + return threddsWmsCatalogue; + } } - // package private for testing - static boolean useCachedCatalogue(String tdsDatasetPath) { - if (containsCachedCatalogue(tdsDatasetPath)) { - // This date last modified will be updated e.g. in the case of an aggregation with a recheckEvery - final long netcdfDatasetLastModified = catalogueCache.get(tdsDatasetPath).wmsCatalogue.getLastModified(); - final long cacheLastModified = catalogueCache.get(tdsDatasetPath).lastModified; - return cacheLastModified >= netcdfDatasetLastModified; + private static NetcdfDataset acquireNetcdfDataset(HttpServletRequest httpServletRequest, + HttpServletResponse httpServletResponse, String tdsDatasetPath) throws IOException { + NetcdfFile ncf = TdsRequestedDataset.getNetcdfFile(httpServletRequest, httpServletResponse, tdsDatasetPath); + if (TdsRequestedDataset.useNetcdfJavaBuilders()) { + return NetcdfDatasets.enhance(ncf, NetcdfDataset.getDefaultEnhanceMode(), null); + } else { + return NetcdfDataset.wrap(ncf, NetcdfDataset.getDefaultEnhanceMode()); + } + } + + public static void showCache(Formatter formatter) { + formatter.format("%nWmsCache:%n"); + formatter.format("numberOfEntries=%d, ", getNumberOfEntries()); + formatter.format("loads=%d, ", getCacheLoads()); + formatter.format("evictionCount=%d ", catalogueCache.stats().evictionCount()); + formatter.format("%nentries:%n"); + for (Map.Entry entry : catalogueCache.asMap().entrySet()) { + formatter.format(" %s%n", entry.getKey()); } - return false; + } + + public static void resetCache() { + catalogueCache.invalidateAll(); + cacheLoads = 0; } // package private for testing static boolean containsCachedCatalogue(String tdsDatasetPath) { - return catalogueCache.containsKey(tdsDatasetPath); + return catalogueCache.asMap().containsKey(tdsDatasetPath); + } + + static long getNumberOfEntries() { + return catalogueCache.size(); + } + + static int getCacheLoads() { + return cacheLoads; } } diff --git a/tds/src/test/java/thredds/server/wms/TestWmsCache.java b/tds/src/test/java/thredds/server/wms/TestWmsCache.java index 5434fb7267..fbac1a6588 100644 --- a/tds/src/test/java/thredds/server/wms/TestWmsCache.java +++ b/tds/src/test/java/thredds/server/wms/TestWmsCache.java @@ -1,6 +1,7 @@ package thredds.server.wms; import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.Truth.assertWithMessage; import java.io.File; import java.io.IOException; @@ -8,9 +9,11 @@ import java.nio.file.Path; import java.nio.file.Paths; import java.nio.file.StandardCopyOption; +import java.util.List; import javax.servlet.ServletException; import org.junit.*; +import org.junit.experimental.categories.Category; import org.junit.rules.TemporaryFolder; import org.junit.runner.RunWith; import org.slf4j.Logger; @@ -21,8 +24,10 @@ import org.springframework.test.context.junit4.SpringJUnit4ClassRunner; import org.springframework.test.context.web.WebAppConfiguration; import java.lang.invoke.MethodHandles; +import ucar.nc2.dataset.NetcdfDatasets; +import ucar.nc2.util.cache.FileCacheIF; +import ucar.unidata.util.test.category.NeedsCdmUnitTest; -@Ignore("TODO: fix WMS cache - tests fail on windows (and occasionally on GH?)") @RunWith(SpringJUnit4ClassRunner.class) @WebAppConfiguration @ContextConfiguration(locations = {"/WEB-INF/applicationContext.xml"}) @@ -58,65 +63,93 @@ public void clearCache() { @Test public void shouldCacheFile() throws IOException, ServletException { - assertThat(ThreddsWmsServlet.containsCachedCatalogue(TEST_PATH)).isFalse(); - getCapabilities(TEST_PATH); - assertThat(ThreddsWmsServlet.containsCachedCatalogue(TEST_PATH)).isTrue(); - assertThat(ThreddsWmsServlet.useCachedCatalogue(TEST_PATH)).isTrue(); + assertAddedToCache(TEST_PATH); + assertUsedCache(TEST_PATH); } // TODO also test updating an S3 file (currently not implemented through MFileS3) @Test public void shouldCacheS3File() throws IOException, ServletException { - assertThat(ThreddsWmsServlet.containsCachedCatalogue(S3_TEST_PATH)).isFalse(); - getCapabilities(S3_TEST_PATH); - assertThat(ThreddsWmsServlet.containsCachedCatalogue(S3_TEST_PATH)).isTrue(); - assertThat(ThreddsWmsServlet.useCachedCatalogue(S3_TEST_PATH)).isTrue(); + assertAddedToCache(S3_TEST_PATH); + assertUsedCache(S3_TEST_PATH); } @Test public void shouldNotUseOutdatedCacheFile() throws IOException, ServletException { - getCapabilities(TEST_PATH); - assertThat(ThreddsWmsServlet.containsCachedCatalogue(TEST_PATH)).isTrue(); - assertThat(ThreddsWmsServlet.useCachedCatalogue(TEST_PATH)).isTrue(); - + assertAddedToCache(TEST_PATH); updateTestFile(); - assertThat(ThreddsWmsServlet.containsCachedCatalogue(TEST_PATH)).isTrue(); - assertThat(ThreddsWmsServlet.useCachedCatalogue(TEST_PATH)).isFalse(); + assertAddedToCache(TEST_PATH); } @Test public void shouldUseUnchangedAggregation() throws IOException, ServletException { - getCapabilities(AGGREGATION_RECHECK_MINUTE_PATH); - assertThat(ThreddsWmsServlet.containsCachedCatalogue(AGGREGATION_RECHECK_MINUTE_PATH)).isTrue(); - assertThat(ThreddsWmsServlet.useCachedCatalogue(AGGREGATION_RECHECK_MINUTE_PATH)).isTrue(); + assertAddedToCache(AGGREGATION_RECHECK_MINUTE_PATH); + assertUsedCache(AGGREGATION_RECHECK_MINUTE_PATH); } @Test public void shouldUseRecheckedButUnchangedAggregation() throws IOException, ServletException { - getCapabilities(AGGREGATION_RECHECK_MSEC_PATH); - assertThat(ThreddsWmsServlet.containsCachedCatalogue(AGGREGATION_RECHECK_MSEC_PATH)).isTrue(); - assertThat(ThreddsWmsServlet.useCachedCatalogue(AGGREGATION_RECHECK_MSEC_PATH)).isTrue(); + assertAddedToCache(AGGREGATION_RECHECK_MSEC_PATH); // Will be rechecked after 1 ms - assertThat(ThreddsWmsServlet.containsCachedCatalogue(AGGREGATION_RECHECK_MSEC_PATH)).isTrue(); - assertThat(ThreddsWmsServlet.useCachedCatalogue(AGGREGATION_RECHECK_MSEC_PATH)).isTrue(); + assertUsedCache(AGGREGATION_RECHECK_MSEC_PATH); } @Test public void shouldNotUseOutdatedAggregation() throws IOException, ServletException { - getCapabilities(AGGREGATION_RECHECK_MSEC_PATH); - assertThat(ThreddsWmsServlet.containsCachedCatalogue(AGGREGATION_RECHECK_MSEC_PATH)).isTrue(); - assertThat(ThreddsWmsServlet.useCachedCatalogue(AGGREGATION_RECHECK_MSEC_PATH)).isTrue(); - + assertAddedToCache(AGGREGATION_RECHECK_MSEC_PATH); updateTestFile(); - assertThat(ThreddsWmsServlet.containsCachedCatalogue(AGGREGATION_RECHECK_MSEC_PATH)).isTrue(); - assertThat(ThreddsWmsServlet.useCachedCatalogue(AGGREGATION_RECHECK_MSEC_PATH)).isFalse(); + assertAddedToCache(AGGREGATION_RECHECK_MSEC_PATH); + } + + @Test + public void shouldNotLockFileInCache() throws IOException, ServletException { + final String filename = "testGridAsPoint.nc"; + final String testPath = "localContent/" + filename; + getCapabilities(testPath); + assertThat(ThreddsWmsServlet.containsCachedCatalogue(testPath)).isTrue(); + + // check file is not locked in netcdf file cache + final FileCacheIF cache = NetcdfDatasets.getNetcdfFileCache(); + final List entries = cache.showCache(); + assertThat(entries.size()).isGreaterThan(0); + final boolean isLocked = entries.stream().filter(e -> e.contains(filename)).anyMatch(e -> e.startsWith("true")); + assertWithMessage(cache.showCache().toString()).that(isLocked).isFalse(); + } + + @Test + @Category(NeedsCdmUnitTest.class) + public void shouldNotLockAggregationInCache() throws IOException, ServletException { + final String testPath = "ExampleNcML/Agg.nc"; + getCapabilities(testPath); + assertThat(ThreddsWmsServlet.containsCachedCatalogue(testPath)).isTrue(); + + // check aggregation is not locked in netcdf file cache + final FileCacheIF cache = NetcdfDatasets.getNetcdfFileCache(); + final List entries = cache.showCache(); + assertThat(entries.size()).isGreaterThan(0); + final boolean isLocked = entries.stream().filter(e -> e.contains(testPath)).anyMatch(e -> e.startsWith("true")); + assertWithMessage(cache.showCache().toString()).that(isLocked).isFalse(); } private void updateTestFile() throws IOException { Files.copy(TEST_FILE, TEMP_FILE, StandardCopyOption.REPLACE_EXISTING); } + private void assertUsedCache(String path) throws ServletException, IOException { + int loads = ThreddsWmsServlet.getCacheLoads(); + getCapabilities(path); + assertThat(ThreddsWmsServlet.containsCachedCatalogue(path)).isTrue(); + assertThat(ThreddsWmsServlet.getCacheLoads()).isEqualTo(loads); + } + + private void assertAddedToCache(String path) throws ServletException, IOException { + int loads = ThreddsWmsServlet.getCacheLoads(); + getCapabilities(path); + assertThat(ThreddsWmsServlet.containsCachedCatalogue(path)).isTrue(); + assertThat(ThreddsWmsServlet.getCacheLoads()).isEqualTo(loads + 1); + } + private void getCapabilities(String path) throws ServletException, IOException { final String uri = "/thredds/wms/" + path; final MockHttpServletRequest request = new MockHttpServletRequest("GET", uri);