Fix circular dependency in Settings initialization

[msfroh]

Description

There was a weird circular class-loading dependency between Settings and Settings.Builder that could result in ClassLoader deadlock if multiple threads trigger class-loading of Settings at the same time.

Related Issues

Resolves #10065

Check List

• New functionality includes testing.
  • All tests pass
• New functionality has been documented.
  • New functionality has javadoc added
• Commits are signed per the DCO using --signoff
• Commit changes are listed out in CHANGELOG.md file (See: Changelog)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: ❌
• URL:
• CommitID: e7b2082
  Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
  Is the failure a flaky test unrelated to your change?

[github-actions]

Compatibility status:

Checks if related components are compatible with change 1c79c83

Incompatible components

Skipped components

Compatible components

Compatible components: [https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/security-analytics.git, https://github.com/opensearch-project/custom-codecs.git, https://github.com/opensearch-project/performance-analyzer.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/neural-search.git]

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: UNSTABLE ❕
• TEST FAILURES:

      1 org.opensearch.remotestore.RemoteStoreStatsIT.testStatsResponseAllShards

• URL: https://build.ci.opensearch.org/job/gradle-check/26148/
• CommitID: e7b2082
  Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[codecov]

Codecov Report

Merging #10194 (1c79c83) into main (d656e3d) will decrease coverage by 0.08%.
Report is 4 commits behind head on main.
The diff coverage is 69.44%.

@@             Coverage Diff              @@
##               main   #10194      +/-   ##
============================================
- Coverage     71.24%   71.17%   -0.08%     
- Complexity    58260    58296      +36     
============================================
  Files          4830     4830              
  Lines        274575   274586      +11     
  Branches      40015    40020       +5     
============================================
- Hits         195629   195434     -195     
- Misses        62570    62799     +229     
+ Partials      16376    16353      -23     

Files	Coverage Δ
.../java/org/opensearch/common/settings/Settings.java	80.62% <100.00%> (ø)
...org/opensearch/index/search/stats/SearchStats.java	82.02% <0.00%> (ø)
...java/org/opensearch/index/shard/IndexingStats.java	86.61% <0.00%> (ø)
...rg/opensearch/repositories/s3/S3BlobContainer.java	79.15% <76.66%> (-1.08%)	⬇️

... and 450 files with indirect coverage changes

[dblock]

LGTM, want to finish it?

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: ❌
• URL:
• CommitID: 7283363
  Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
  Is the failure a flaky test unrelated to your change?

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: ❌
• URL:
• CommitID: 7283363
  Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
  Is the failure a flaky test unrelated to your change?

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE ❌
• URL: https://build.ci.opensearch.org/job/gradle-check/26488/
• CommitID: 7283363
  Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
  Is the failure a flaky test unrelated to your change?

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE ❌
• URL: https://build.ci.opensearch.org/job/gradle-check/26492/
• CommitID: 7283363
  Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
  Is the failure a flaky test unrelated to your change?

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: SUCCESS ✅
• URL: https://build.ci.opensearch.org/job/gradle-check/26525/
• CommitID: 1c79c83

[msfroh]

I was able to reproduce the deadlock with the following simple class:

public class SettingsMain {
    public static void main(String[] args) {
        new Thread(Settings.Builder.EMPTY_SETTINGS::getSecureSettings).start();
        Settings.builder();
    }
}

When I run that, the process just hangs. (It's essentially the idea discussed in https://www.farside.org.uk/201510/deadlocks_in_java_class_initialisation.)

With the code change from this PR, it doesn't hang -- it just completes immediately.

[msfroh]

Unfortunately, since Settings are used throughout the codebase, I can't add it as a unit test. Maybe a dedicated Gradle task?

My preference (partly driven by laziness) is just apply the fix as is -- these circular static initializer dependencies are tricky to catch in automated way.

[reta]

My preference (partly driven by laziness) is just apply the fix as is -- these circular static initializer dependencies are tricky to catch in automated way.

This is tricky to test, if JVM hangs we should have a way to terminate it properly by failing the test, not very straightforward since this is a deadlock.

[reta]

@dblock good with that? (skipping the test part)

[dblock]

YOLO I hit merge. Thanks @msfroh and @reta.