[GitHub] [solr] sonatype-lift[bot] commented on a change in pull request #488: SOLR-15883 Fix a problem in OpenExchangeRatesOrgProvider: introduce a parameter so that it downloads currency rates while indexing not while searching

[GitBox <gi...@apache.org>]

sonatype-lift[bot] commented on a change in pull request #488:
URL: https://github.com/apache/solr/pull/488#discussion_r785339009



##########
File path: solr/core/src/java/org/apache/solr/schema/OpenExchangeRatesOrgProvider.java
##########
@@ -135,31 +144,47 @@ public String toString() {
     return rates.getRates().keySet();
   }
 
-  @Override
-  @SuppressWarnings("resource")
-  public boolean reload() throws SolrException {
-    InputStream ratesJsonStream = null;
+  public void reloadNow() throws SolrException {
     try {
       log.debug("Reloading exchange rates from {}", ratesFileLocation);
-      try {
-        ratesJsonStream = (new URL(ratesFileLocation)).openStream();
-      } catch (Exception e) {
-        ratesJsonStream = resourceLoader.openResource(ratesFileLocation);
-      }
-        
-      rates = new OpenExchangeRates(ratesJsonStream);
-      return true;
+
+      // We set the timestamp based on the monotonic time not the time from openexchangerates.com because
+      // in the reload() method we will be comparing it to the monotonic time. If we took the time
+      // from openexchangerates.com and the timestamp was off or the system clock was set to a different time, we could
+      // be refreshing the exchange rates too often (even on every search request) or too rarely. Also, it's necessary
+      // to set the timestamp to the current time and to do it before the actual reload, so in the case
+      // when the openexchangerates.com server is down for more than 60 minutes, we don't try to refresh the rates
+      // on every search request.
+      lastReloadTimestamp = TimeUnit.NANOSECONDS.toSeconds(System.nanoTime());
+
+      rates = new OpenExchangeRates();
+      log.debug("Successfully reloaded exchange rates from {}", ratesFileLocation);
     } catch (Exception e) {
-      throw new SolrException(ErrorCode.SERVER_ERROR, "Error reloading exchange rates", e);
+      log.error("Error reloading exchange rates", e);
     } finally {
-      if (ratesJsonStream != null) {
-        try {
-          ratesJsonStream.close();
-        } catch (IOException e) {
-          throw new SolrException(ErrorCode.SERVER_ERROR, "Error closing stream", e);
-        }
+      reloading = false;
+    }
+  }
+
+  @Override
+  public boolean reload() throws SolrException {
+    if ((lastReloadTimestamp + refreshIntervalSeconds) >= TimeUnit.NANOSECONDS.toSeconds(System.nanoTime())) {
+      return true;
+    }
+
+    synchronized (this) {
+      if (!reloading) {
+        log.debug("Refresh interval has expired. Refreshing exchange rates (in a separate thread).");
+        reloading = true;
+        executorService.submit(this::reloadNow);

Review comment:
       *FutureReturnValueIgnored:*  Return value of methods returning Future must be checked. Ignoring returned Futures suppresses exceptions thrown from the code that completes the Future. [(details)](https://errorprone.info/bugpattern/FutureReturnValueIgnored)
   (at-me [in a reply](https://help.sonatype.com/lift/talking-to-lift) with `help` or `ignore`)

##########
File path: solr/core/src/java/org/apache/solr/schema/OpenExchangeRatesOrgProvider.java
##########
@@ -135,31 +144,47 @@ public String toString() {
     return rates.getRates().keySet();
   }
 
-  @Override
-  @SuppressWarnings("resource")
-  public boolean reload() throws SolrException {
-    InputStream ratesJsonStream = null;
+  public void reloadNow() throws SolrException {
     try {
       log.debug("Reloading exchange rates from {}", ratesFileLocation);
-      try {
-        ratesJsonStream = (new URL(ratesFileLocation)).openStream();
-      } catch (Exception e) {
-        ratesJsonStream = resourceLoader.openResource(ratesFileLocation);
-      }
-        
-      rates = new OpenExchangeRates(ratesJsonStream);
-      return true;
+
+      // We set the timestamp based on the monotonic time not the time from openexchangerates.com because
+      // in the reload() method we will be comparing it to the monotonic time. If we took the time
+      // from openexchangerates.com and the timestamp was off or the system clock was set to a different time, we could
+      // be refreshing the exchange rates too often (even on every search request) or too rarely. Also, it's necessary
+      // to set the timestamp to the current time and to do it before the actual reload, so in the case
+      // when the openexchangerates.com server is down for more than 60 minutes, we don't try to refresh the rates
+      // on every search request.
+      lastReloadTimestamp = TimeUnit.NANOSECONDS.toSeconds(System.nanoTime());
+
+      rates = new OpenExchangeRates();
+      log.debug("Successfully reloaded exchange rates from {}", ratesFileLocation);
     } catch (Exception e) {
-      throw new SolrException(ErrorCode.SERVER_ERROR, "Error reloading exchange rates", e);
+      log.error("Error reloading exchange rates", e);
     } finally {
-      if (ratesJsonStream != null) {
-        try {
-          ratesJsonStream.close();
-        } catch (IOException e) {
-          throw new SolrException(ErrorCode.SERVER_ERROR, "Error closing stream", e);
-        }
+      reloading = false;
+    }
+  }
+
+  @Override
+  public boolean reload() throws SolrException {
+    if ((lastReloadTimestamp + refreshIntervalSeconds) >= TimeUnit.NANOSECONDS.toSeconds(System.nanoTime())) {

Review comment:
       *THREAD_SAFETY_VIOLATION:*  Read/Write race. Non-private method `OpenExchangeRatesOrgProvider.reload()` reads without synchronization from `this.refreshIntervalSeconds`. Potentially races with write in method `OpenExchangeRatesOrgProvider.init(...)`.
    Reporting because this access may occur on a background thread.
   (at-me [in a reply](https://help.sonatype.com/lift/talking-to-lift) with `help` or `ignore`)




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org