SOLR-16427: Enable error-prone ClassInitializationDeadlock rule #2881

malliaridis · 2024-11-21T00:37:54Z

https://issues.apache.org/jira/browse/SOLR-16427

Description

With the ClassInitializationDeadlock rule error-prone checks for possible class initialization deadlocks. See error-prone documentation for details about the problem this rule is fixing.

Solution

Following the suggestions of error-prone, the possible deadlocks are resolved by introducing separate classes for static members.

You may review commits individually for a better overview.

This PR is not backwards compatible and public static members are moved to separate classes, potentially breaking third-party code that uses these static members.

Tests

Tests were not altered, only existing references to static members are updated.

Checklist

Please review the following and check all that apply:

I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
I have created a Jira issue and added the issue ID to my pull request title.
I have given Solr maintainers access to contribute to my PR branch. (optional but recommended, not available for branches on forks living under an organisation)
I have developed this patch against the main branch.
I have run ./gradlew check.
I have added tests for my changes.
I have added documentation for the Reference Guide

dsmiley

First, thanks for working on this. It's been a problem for Lucene & Solr over the years. @uschindler , you have always been involved in this topic so I'm tagging you for any additional review.

I suggest limiting this PR to be only main, without any concern for deprecating stuff. By and large, you were deprecating very internal things.

TimeSource has annoyed me; I filed SOLR-17573 to improve that (albeit may be controversial).

dsmiley · 2024-11-24T19:52:54Z

solr/core/src/java/org/apache/solr/core/SolrCore.java

-      qParserPlugins.init(QParserPlugin.standardPlugins, this);
-      valueSourceParsers.init(ValueSourceParser.standardValueSourceParsers, this);
-      transformerFactories.init(TransformerFactory.defaultFactories, this);
+      qParserPlugins.init(QParserPlugins.standardPlugins, this);


These 3 registries only exist for SolrCore registration. You could move them to the same package and make them not public.

But longer term (or soon if you are to do the work), I'd rather our built-in plugins be auto-discovered and registered via embracing java.util.ServiceLoader. Then we would have no static registries. -- CC @janhoy

Some tests outside the core package currently use these registries, including:

org.apache.solr.cloud.TestRandomFlRTGCloud (ref. transformers)

org.apache.solr.search.QueryEqualityTest (ref. value source parsers and query parser plugins)

org.apache.solr.search.TestStandardQParsers (ref. query parser plugins)

org.apache.solr.search.SignificantTermsQParserPluginTest (ref. query parser plugins)

From the logical grouping perspective it makes sense to me to keep them in the current packages. From the visibility perspective (public API) I would prefer to make them package-private / internal.

Perhaps its best to keep it as is for now until we introduce an auto-discovery feature?

solr/core/src/java/org/apache/solr/response/transform/TransformerFactory.java

dsmiley · 2024-11-24T20:02:01Z

solr/core/src/java/org/apache/solr/search/FunctionQParser.java

@@ -398,7 +398,7 @@ protected ValueSource parseValueSource(int flags) throws SyntaxError {
    if ((ch >= '0' && ch <= '9') || ch == '.' || ch == '+' || ch == '-') {
      Number num = sp.getNumber();
      if (num instanceof Long) {
-        valueSource = new ValueSourceParser.LongConstValueSource(num.longValue());
+        valueSource = new ValueSourceParsers.LongConstValueSource(num.longValue());


it appears inconsistent with the lower two lines; can you static import this? Or move to top level if the below ones are (consistency)

This one is actually interesting. The other two value sources are from Lucene, LongConstValueSource is from Solr, which is why this is inconsistent. I could add an import, but perhaps something else could be considered here?

Would ConstValueSource be the equivalent in Lucene for long values? Because I can only find ConstValueSource for double in Lucene.

solr/core/src/java/org/apache/solr/search/QParserPlugin.java

solr/core/src/java/org/apache/solr/search/ValueSourceParser.java

dsmiley · 2024-11-24T20:12:01Z

solr/solrj/src/java/org/apache/solr/common/cloud/DocRouter.java

+   * @deprecated Use {@link DocRouters#DEFAULT_NAME} instead.
+   */
+  @Deprecated(since = "9.8", forRemoval = true)
+  public static final String DEFAULT_NAME = DocRouters.DEFAULT_NAME;


WDYT about keeping this as a constant string? Then almost nobody would have a need to reference the new DocRouters.

I considered this an option, but thought that it would be better to keep DEFAULT (DocRouter) and DEFAULT_NAME (String) in the same class. So moving DEFAULT out would also affect DEFAULT_NAME.

Since I also moved out the getDocRouter(routerName) method, we still have a few more references to the new DocRouters. But perhaps that is also not necessary?

Keeping getDocRouter(routerName) would probably be another case of class initialization deadlock, so I moved forward and removed the deprecation and kept DocRouters.DEFAULT_NAME, so that it aligns with the class reference of DocRouters.DEFAULT.

dsmiley · 2024-11-24T20:16:55Z

solr/solrj/src/java/org/apache/solr/common/MapWriters.java

+
+import java.util.Collections;
+
+public class MapWriters {


just for EMPTY feels sad...
Instead, I could imagine MapWriter.empty() that references an EMPTY constant declared on MapWriterMap

Moving EMPTY to MapWriterMap and providing a static MapWriter#empty() method looks like another possible case of ClassInitializationDeadlock. Allthough it is not identified by error-prone, I believe it is similar to the examples. But maybe I am wrong on that?

malliaridis · 2024-11-25T15:57:11Z

Thanks for the review @dsmiley. I am not familiar enough with the modified classes to know which is cnosidered internal, so your input is very valuable to me. I generally try to follow the rule "anything that is public requires at least one major version of deprecation before removing it". But since it is considered internal, I will move forward and cleanup the deprecations. :)

Note that TransformerFactories is now immutable.

…ialization-deadlock # Conflicts: # solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBHttp2SolrClientIntegrationTest.java

github-actions · 2025-01-31T00:00:30Z

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 [email protected] 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!

…ialization-deadlock # Conflicts: # solr/core/src/test/org/apache/solr/handler/admin/IndexSizeEstimatorTest.java # solr/solrj/src/java/org/apache/solr/common/cloud/DocRouter.java

uschindler

I have not checked all details, but in general the approach to use a static final class with plural s is a workable approach, although it's not backwards compatible. But it is only way for static final constants.

If you have accessor methods, you can use the holder pattern used in Lucene.

uschindler · 2025-02-09T13:02:16Z

Please be still careful to not use the plural s classes during initialization on the original class. This would reintroduce the problems. I wasn't able to check all those patterns, but I hope the error probe checker would gave found those, too.

malliaridis · 2025-02-09T13:41:21Z

Thanks for your feedback @uschindler 😄

Please be still careful to not use the plural s classes during initialization on the original class. This would reintroduce the problems.

I believe this is not the case, and I hope error-prone would identify such cases.

If you have accessor methods, you can use the holder pattern used in Lucene.

I haven't used this pattern before. Would TimeSources.get(type) (previously TimeSource.get(type)) be such a scenario where this pattern could be used?

I guess in cases where the instances are public and directly accessed or added to a list with static { ... } the holder pattern cannot be used?

uschindler · 2025-02-09T15:25:11Z

Thanks for your feedback @uschindler 😄

Please be still careful to not use the plural s classes during initialization on the original class. This would reintroduce the problems.

I believe this is not the case, and I hope error-prone would identify such cases.

OK.

If you have accessor methods, you can use the holder pattern used in Lucene.

I haven't used this pattern before. Would TimeSources.get(type) (previously TimeSource.get(type)) be such a scenario where this pattern could be used?

The pattern can be seen here when codec initialization is done. Lucene has a default Codec, which is a subclass of the Codec class. The Codec class has a method Codec#getDefault() which returns the static default. The initialization is delayed by a Holder pattern:

https://github.com/apache/lucene/blob/30d18dfbe1e67bed2d4550b2bc20b9b86ce9781c/lucene/core/src/java/org/apache/lucene/codecs/Codec.java#L40-L59

Here it is called:

https://github.com/apache/lucene/blob/30d18dfbe1e67bed2d4550b2bc20b9b86ce9781c/lucene/core/src/java/org/apache/lucene/codecs/Codec.java#L139-L147

Basically the trick is similar to your's but has more checks to really prevent codec subclasses to get the default during initialization phase (that's why the null checks are there).

I guess in cases where the instances are public and directly accessed or added to a list with static { ... } the holder pattern cannot be used?

No, that's the problem of most classes seen in this PR. It is impossible to use that where the superclass has a static final field to one of the subclasses. You need to break backwards compatibility.

dsmiley · 2025-02-09T18:44:45Z

Looking back at this a bit briefly. In general these plural form classes should be very unreferenced except when the registry itself is needed (should be rare). These classes should be considered @lucene.internal (we don't have a Solr variation yet) as we pursue ServiceProviderInterface. Instead of DocRouters.DEFAULT, add DocRouter.getDefault().

malliaridis added 7 commits November 20, 2024 22:15

Enable ClassInitializationDeadlock checks

5b8526d

Fix ClassInitializationDeadlock warning for TimeSource

eaf4d70

Fix ClassInitializationDeadlock warning for DocRouter

84c5ac4

Fix ClassInitializationDeadlock warning for MapWriter

5dd13bb

Fix ClassInitializationDeadlock warning for QParserPlugin

cc7249a

Fix ClassInitializationDeadlock warning for ValueSourceParser

1373527

Fix ClassInitializationDeadlock warning for TransformerFactory

dff22d4

github-actions bot added test-framework jetty-server client:solrj module:hdfs module:jwt-auth tool:build tests cat:search cat:cloud cat:index cat:api cat:schema cat:security module:scripting labels Nov 21, 2024

dsmiley self-requested a review November 24, 2024 20:48

dsmiley reviewed Nov 24, 2024

View reviewed changes

malliaridis added 5 commits November 27, 2024 13:55

Use Map.ofEntries() for simplifying static initialization

b0d1aa2

Note that TransformerFactories is now immutable.

Remove deprecated code

29c52ca

Remove redundant semicolons

a9c9402

Merge remote-tracking branch 'apache/main' into SOLR-16427/class-init…

9aaa7df

…ialization-deadlock # Conflicts: # solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBHttp2SolrClientIntegrationTest.java

Update CHANGES.txt

b702a82

malliaridis marked this pull request as ready for review December 1, 2024 14:52

github-actions bot added the stale PR not updated in 60 days label Jan 31, 2025

Merge remote-tracking branch 'apache/main' into SOLR-16427/class-init…

b34abd5

…ialization-deadlock # Conflicts: # solr/core/src/test/org/apache/solr/handler/admin/IndexSizeEstimatorTest.java # solr/solrj/src/java/org/apache/solr/common/cloud/DocRouter.java

malliaridis force-pushed the SOLR-16427/class-initialization-deadlock branch from a1e962f to b34abd5 Compare February 9, 2025 12:47

uschindler approved these changes Feb 9, 2025

View reviewed changes

github-actions bot removed the stale PR not updated in 60 days label Feb 10, 2025

dsmiley mentioned this pull request Feb 10, 2025

SOLR-17662 SPI: TransformerFactory/DocTransformer #3172

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SOLR-16427: Enable error-prone ClassInitializationDeadlock rule #2881

SOLR-16427: Enable error-prone ClassInitializationDeadlock rule #2881

malliaridis commented Nov 21, 2024 •

edited

Loading

dsmiley left a comment

dsmiley Nov 24, 2024

malliaridis Nov 27, 2024

dsmiley Nov 24, 2024

malliaridis Nov 27, 2024

dsmiley Nov 24, 2024

malliaridis Nov 27, 2024

malliaridis Nov 27, 2024

dsmiley Nov 24, 2024

malliaridis Nov 27, 2024

malliaridis commented Nov 25, 2024

github-actions bot commented Jan 31, 2025

uschindler left a comment

uschindler commented Feb 9, 2025

malliaridis commented Feb 9, 2025

uschindler commented Feb 9, 2025

dsmiley commented Feb 9, 2025

SOLR-16427: Enable error-prone ClassInitializationDeadlock rule #2881

Are you sure you want to change the base?

SOLR-16427: Enable error-prone ClassInitializationDeadlock rule #2881

Conversation

malliaridis commented Nov 21, 2024 • edited Loading

Description

Solution

Tests

Checklist

dsmiley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malliaridis commented Nov 25, 2024

github-actions bot commented Jan 31, 2025

uschindler left a comment

Choose a reason for hiding this comment

uschindler commented Feb 9, 2025

malliaridis commented Feb 9, 2025

uschindler commented Feb 9, 2025

dsmiley commented Feb 9, 2025

malliaridis commented Nov 21, 2024 •

edited

Loading