Mike Drob commented on SOLR-9524:

Thanks for looking. I agree that SOLR-9506 would be a good improvement too.

The reason I didn't use computeIfAbsent is computing fingerprint could throw an 
exception. Code to account for exception looks pretty ugly.

It also seems like some changes to the original patch, I dont remember using 
The CHM was added in commit 37ae065 by Noble.

I think that the ugliness of the code around exception handling here is a fine 
tradeoff for the correctness issue that we've already seen and the performance 
issue of locking on the whole map. If you have 5 fingerprints to calculate, 
then the current code will need 10 seconds total since each calculation can 
only happen serially. Letting computeIfAbsent manage the concurrent access for 
us seems much much better.

> SolrIndexSearcher.getIndexFingerprint uses dubious sunchronization
> ------------------------------------------------------------------
>                 Key: SOLR-9524
>                 URL: https://issues.apache.org/jira/browse/SOLR-9524
>             Project: Solr
>          Issue Type: Bug
>      Security Level: Public(Default Security Level. Issues are Public) 
>    Affects Versions: 6.3
>            Reporter: Mike Drob
>         Attachments: SOLR-9524.patch
> In SOLR-9310 we added more code that does some fingerprint caching in 
> SolrIndexSearcher. However, the synchronization looks like it could be made 
> more efficient and may have issues with correctness.
> https://github.com/apache/lucene-solr/blob/branch_6x/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java#L2371-L2385
> Some of the issues:
> * Double checked locking needs use of volatile variables to ensure proper 
> memory semantics.
> * sync on a ConcurrentHashMap is usually a code smell

This message was sent by Atlassian JIRA

To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org

Reply via email to