[
https://issues.apache.org/jira/browse/LUCENE-10292?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17529223#comment-17529223
]
Dawid Weiss commented on LUCENE-10292:
--------------------------------------
Thanks Chris. I'm still not sure whether these tests make sense without
explicitly stating that build() can be called on Lookup to dynamically (and
concurrently) replace its internals... For example, FSTCompletionLookup:
{code}
// The two FSTCompletions share the same automaton.
this.higherWeightsCompletion = builder.build();
this.normalCompletion =
new FSTCompletion(higherWeightsCompletion.getFST(), false,
exactMatchFirst);
this.count = newCount;
{code}
none of these fields are volatile or under a monitor, so no guaranteed flush
occurs anywhere. I understand eventually they'll get consistent by piggybacking
on some other synchronization/ memfence but it's weird to rely on this
behavior. I think it'd be a much more user-friendly API if Lookup was actually
detached entirely from its build process (for example by replacing the current
build method with a builder() that would return a new immutable Lookup
instance). This would be less confusing and would also allow for a cleaner
implementation (no synchronization at all required - just regular assignments,
maybe even with final fields).
I'm not saying this should be implemented here - perhaps it's worth a new issue
to do this refactoring.
Separately from the above, if the test fails, it'll leak threads - this:
+ acquireOnNext.acquireUninterruptibly();
literally blocks forever. It should be replaced with a try/catch that rethrows
an unchecked exception when the iterator thread is interrupted.
> AnalyzingInfixSuggester thread safety: lookup() fails during (re)build()
> ------------------------------------------------------------------------
>
> Key: LUCENE-10292
> URL: https://issues.apache.org/jira/browse/LUCENE-10292
> Project: Lucene - Core
> Issue Type: Bug
> Reporter: Chris M. Hostetter
> Assignee: Chris M. Hostetter
> Priority: Major
> Fix For: 10.0 (main), 9.2
>
> Attachments: LUCENE-10292-1.patch, LUCENE-10292-2.patch,
> LUCENE-10292-3.patch, LUCENE-10292.patch
>
>
> I'm filing this based on anecdotal information from a Solr user w/o
> experiencing it first hand (and I don't have a test case to demonstrate it)
> but based on a reading of the code the underlying problem seems self
> evident...
> With all other Lookup implementations I've examined, it is possible to call
> {{lookup()}} regardless of whether another thread is concurrently calling
> {{build()}} – in all cases I've seen, it is even possible to call
> {{lookup()}} even if {{build()}} has never been called: the result is just an
> "empty" {{List<LookupResult>}}
> Typically this is works because the {{build()}} method uses temporary
> datastructures until it's "build logic" is complete, at which point it
> atomically replaces the datastructures used by the {{lookup()}} method. In
> the case of {{AnalyzingInfixSuggester}} however, the {{build()}} method
> starts by closing & null'ing out the {{protected SearcherManager
> searcherMgr}} (which it only populates again once it's completed building up
> it's index) and then the lookup method starts with...
> {code:java}
> if (searcherMgr == null) {
> throw new IllegalStateException("suggester was not built");
> }
> {code}
> ... meaning it is unsafe to call {{AnalyzingInfixSuggester.lookup()}} in any
> situation where another thread may be calling
> {{AnalyzingInfixSuggester.build()}}
--
This message was sent by Atlassian Jira
(v8.20.7#820007)
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
