You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "magibney (via GitHub)" <gi...@apache.org> on 2023/03/22 13:23:38 UTC
[GitHub] [solr] magibney opened a new pull request, #1481: SOLR-16707: Avoid "Recursive update" ISE in non-async cache `computeIfAbsent()`
magibney opened a new pull request, #1481:
URL: https://github.com/apache/solr/pull/1481
an alternative to #1480
See: SOLR-16707
--
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
[GitHub] [solr] magibney commented on pull request #1481: SOLR-16707: Avoid "Recursive update" ISE in non-async cache `computeIfAbsent()`
Posted by "magibney (via GitHub)" <gi...@apache.org>.
magibney commented on PR #1481:
URL: https://github.com/apache/solr/pull/1481#issuecomment-1479685970
I'm thinking this warrants a "bugfix" CHANGES.txt entry?
--
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
[GitHub] [solr] dsmiley commented on pull request #1481: SOLR-16707: Avoid "Recursive update" ISE in non-async cache `computeIfAbsent()`
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #1481:
URL: https://github.com/apache/solr/pull/1481#issuecomment-1660236253
Looking back at this, the problem is that the cache implementation can't recursively compute new cache entries. Given this situation, couldn't we use a ThreadLocal boolean to indicate if we're recursively being invoked and make different decisions based on this? If recursive invocation, need to compute the query prior to the computeIfAbsent call. The vast majority of invocations will not be recursive though.
--
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
[GitHub] [solr] magibney commented on pull request #1481: SOLR-16707: Avoid "Recursive update" ISE in non-async cache `computeIfAbsent()`
Posted by "magibney (via GitHub)" <gi...@apache.org>.
magibney commented on PR #1481:
URL: https://github.com/apache/solr/pull/1481#issuecomment-1658564753
This fell through the cracks; circling back now, I plan to merge this within the next couple of days, pending any objections/feedback. cc @dsmiley , @mkhludnev.
--
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
[GitHub] [solr] magibney commented on pull request #1481: SOLR-16707: Avoid "Recursive update" ISE in non-async cache `computeIfAbsent()`
Posted by "magibney (via GitHub)" <gi...@apache.org>.
magibney commented on PR #1481:
URL: https://github.com/apache/solr/pull/1481#issuecomment-1660841626
This sounds good; I had been thinking to not optimize the `async=false` case, and to instead move toward deprecating it. But I like this; it's straightforward, and will also provide a template for a simple way for plugin SolrCache impl's to handle this situation without requiring them to implement full-blown async support.
--
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
[GitHub] [solr] magibney commented on pull request #1481: SOLR-16707: Avoid "Recursive update" ISE in non-async cache `computeIfAbsent()`
Posted by "magibney (via GitHub)" <gi...@apache.org>.
magibney commented on PR #1481:
URL: https://github.com/apache/solr/pull/1481#issuecomment-1675147534
@dsmiley thanks again for your suggestion; it's implemented in 3fb684f0291b508e7cbca3ad964dbe779211ff8d if you care to take a look!
--
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
Re: [PR] SOLR-16707: Avoid "Recursive update" ISE in non-async cache `computeIfAbsent()` [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #1481:
URL: https://github.com/apache/solr/pull/1481#issuecomment-2400527144
@magibney you seemed so close to this nice change being merged
--
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
Re: [PR] SOLR-16707: Avoid "Recursive update" ISE in non-async cache `computeIfAbsent()` [solr]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #1481:
URL: https://github.com/apache/solr/pull/1481#issuecomment-1947529683
This PR had no visible activity in the past 60 days, labeling it as stale. Any new activity will remove the stale label. To attract more reviewers, please tag someone or notify the dev@solr.apache.org mailing list. Thank you for your contribution!
--
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
Re: [PR] SOLR-16707: Avoid "Recursive update" ISE in non-async cache `computeIfAbsent()` [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1481:
URL: https://github.com/apache/solr/pull/1481#discussion_r1405553264
##########
solr/core/src/java/org/apache/solr/search/CaffeineCache.java:
##########
@@ -98,6 +98,13 @@ public class CaffeineCache<K, V> extends SolrCacheBase
private int maxIdleTimeSec;
private boolean cleanupThread;
private boolean async;
+ private final ThreadLocal<boolean[]> detectRecursion =
+ new ThreadLocal<>() {
+ @Override
+ protected boolean[] initialValue() {
Review Comment:
Why a boolean array; why not a Boolean? We can easily change the ThreadLocal value; it's not immutable.
Also, it's less code to use ThreadLocal.[withInitial](https://www.tabnine.com/code/java/methods/java.lang.ThreadLocal/withInitial)
##########
solr/core/src/java/org/apache/solr/search/CaffeineCache.java:
##########
@@ -269,7 +289,38 @@ public V computeIfAbsent(K key, IOFunction<? super K, ? extends V> mappingFuncti
});
} catch (UncheckedIOException e) {
throw e.getCause();
+ } catch (RecursionException ignored) {
+ // recursive invocation detected; fallback to get-then-put
+ }
+
Review Comment:
At this line, a comment saying "Recursive update detected; fall back on get-then-put" would clarify the overall flow of this complicated method.
--
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
Re: [PR] SOLR-16707: Avoid "Recursive update" ISE in non-async cache `computeIfAbsent()` [solr]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #1481:
URL: https://github.com/apache/solr/pull/1481#issuecomment-2533260382
This PR has had no activity for 60 days and is now labeled as stale. Any new activity will remove the stale label. To attract more reviewers, please tag people who might be familiar with the code area and/or notify the dev@solr.apache.org mailing list. To exempt this PR from being marked as stale, make it a draft PR or add the label "exempt-stale". If left unattended, this PR will be closed after another 60 days of inactivity. Thank you for your contribution!
--
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
[GitHub] [solr] magibney commented on a diff in pull request #1481: SOLR-16707: Avoid "Recursive update" ISE in non-async cache `computeIfAbsent()`
Posted by "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
Re: [PR] SOLR-16707: Avoid "Recursive update" ISE in non-async cache `computeIfAbsent()` [solr]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #1481: SOLR-16707: Avoid "Recursive update" ISE in non-async cache `computeIfAbsent()`
URL: https://github.com/apache/solr/pull/1481
--
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
[GitHub] [solr] dsmiley commented on a diff in pull request #1481: SOLR-16707: Avoid "Recursive update" ISE in non-async cache `computeIfAbsent()`
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1481:
URL: https://github.com/apache/solr/pull/1481#discussion_r1145419650
##########
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:
Computing the value outside the cache means a potentially expensive query that is requested concurrently will be computed redundantly. I think that's the value that was added previously, now defeated. Maybe we can get this behavior back by always doing async mode in 10?
CC @madrob who I think added this
--
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
[GitHub] [solr] dsmiley commented on pull request #1481: SOLR-16707: Avoid "Recursive update" ISE in non-async cache `computeIfAbsent()`
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #1481:
URL: https://github.com/apache/solr/pull/1481#issuecomment-1660819926
You could even modify the logic so we optimistically assume non-recursion (the fast path). if the lambda in compute-if-absent sees we aren't so lucky, it could throw an exception to bail out of the computeIfAbsent call. Catch it outside, and do the fallback logic.
--
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
Re: [PR] SOLR-16707: Avoid "Recursive update" ISE in non-async cache `computeIfAbsent()` [solr]
Posted by "magibney (via GitHub)" <gi...@apache.org>.
magibney commented on PR #1481:
URL: https://github.com/apache/solr/pull/1481#issuecomment-2407509951
Ah, thanks for not letting this slip through the cracks; I'll follow up on this shortly
--
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
Re: [PR] SOLR-16707: Avoid "Recursive update" ISE in non-async cache `computeIfAbsent()` [solr]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #1481:
URL: https://github.com/apache/solr/pull/1481#issuecomment-2646664503
This PR is now closed due to 60 days of inactivity after being marked as stale. Re-opening this PR is still possible, in which case it will be marked as active again.
--
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