[GitHub] [solr] magibney commented on a diff in pull request #1481: SOLR-16707: Avoid "Recursive update" ISE in non-async cache `computeIfAbsent()`

["magibney (via GitHub)" <gi...@apache.org>]

magibney commented on code in PR #1481:
URL: https://github.com/apache/solr/pull/1481#discussion_r1145441657


##########
solr/core/src/java/org/apache/solr/search/CaffeineCache.java:
##########
@@ -250,26 +249,35 @@ public V computeIfAbsent(K key, IOFunction<? super K, ? extends V> mappingFuncti
       return computeAsync(key, mappingFunction);
     }
 
-    try {
-      return cache.get(
-          key,
-          k -> {
-            V value;
-            try {
-              value = mappingFunction.apply(k);
-            } catch (IOException e) {
-              throw new UncheckedIOException(e);
-            }
-            if (value == null) {
-              return null;
-            }
-            recordRamBytes(key, null, value);
-            inserts.increment();
-            return value;
-          });
-    } catch (UncheckedIOException e) {
-      throw e.getCause();
+    /*
+    Synchronous caches must effectively use get-then-put under the hood in place of
+    `computeIfAbsent()`-type behavior. A number of Solr queries (and consequently,
+    `mappingFunction`s passed into this method) would modify the cache recursively.
+    This is supported in `async` mode, but when `async==false`, it may yield
+    "IllegalStateException: Recursive update" (see SOLR-16707).
+     */
+    V cached = cache.getIfPresent(key);
+    if (cached != null) {
+      return cached;
+    }
+    final V computed = mappingFunction.apply(key);

Review Comment:
   Oh 100%, no doubt. In fact I was pretty clear about being in favor of removing the possibility of non-async in [comments on the related issue](https://issues.apache.org/jira/browse/SOLR-16707?focusedCommentId=17703668#comment-17703668).
   
   But until we can deprecate/remove that option, we have two options to continue to support `async=false`, both not great:
   1. document that users need to avoid join queries, avoid certain configurations of range queries and boolean queries, and ?? maybe other things?, or
   2. revert the behavior of `async=false` to what it would have been pre-SOLR-15555, and have everything work, albeit with possible double-computation.
   
   This PR goes the second route. Which, faced with the tradeoff that route implies for `async=false`, makes one wonder why anyone would ever configure `async=false`? But then we're [back to the question](https://issues.apache.org/jira/browse/SOLR-16707?focusedCommentId=17702769#comment-17702769) of whether there's any benefit to keeping `async=false` as an option at all.
   
   In the meantime, this PR will make things work for users who (for whatever reason) enable this option, and it will also stop the unhelpful test failures.



-- 
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