Fix WMS netcdf file locking issue

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();

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() {

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

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);
+  }
 }

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<String, CachedWmsCatalogue> catalogueCache = new HashMap<>();
-
-  static void resetCache() {
-    catalogueCache.clear();
-  }
+  private static final Cache<String, CachedWmsCatalogue> 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<String, CachedWmsCatalogue> 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;
   }
 }