Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Parts of Rest High-Level Client not thread-safe #10065

Closed
dennisoelkers opened this issue Sep 15, 2023 · 3 comments · Fixed by #10194
Closed

[BUG] Parts of Rest High-Level Client not thread-safe #10065

dennisoelkers opened this issue Sep 15, 2023 · 3 comments · Fixed by #10194
Labels
bug Something isn't working Clients Clients within the Core repository such as High level Rest client and low level client v2.11.0 Issues and PRs related to version 2.11.0 v3.0.0 Issues and PRs related to version 3.0.0

Comments

@dennisoelkers
Copy link

dennisoelkers commented Sep 15, 2023

Describe the bug

We were working on an issue in Graylog today when we found out that it is a deadlock between two threads which are stuck in the OpenSearch Rest High-Level Client. The underlying issue is that we have two threads trying to perform (different!) requests to OpenSearch at the same time, which are implicitly making use of the Settings class. This class is inherently unsafe in multi-threaded usage, due to its use of static variables/static Builder class/the circular reference between the outer class and the static nested class.

Our pragmatic fix was to make sure that both threads are started at different times, so initialization of the Settings class through thread 1 should be done when thread 2 starts, mitigating the problem for us. This is the related PR explaining and working around the issue.

It would be better though, if you would restructure the class to remove any of the factors causing a potential deadlock when it is used in two different threads simultaneous.

To Reproduce
Steps to reproduce the behavior:

  1. Go to '...'
  2. Click on '....'
  3. Scroll down to '....'
  4. See error

Expected behavior
A clear and concise description of what you expected to happen.

Plugins
Please list all plugins currently enabled.

Screenshots
If applicable, add screenshots to help explain your problem.

Host/Environment (please complete the following information):

  • OS: [e.g. iOS]
  • Version [e.g. 22]

Additional context
Add any other context about the problem here.

@dennisoelkers dennisoelkers added bug Something isn't working untriaged labels Sep 15, 2023
@dblock
Copy link
Member

dblock commented Sep 18, 2023

@dennisoelkers Is there a way you can use a different transport instead of trying to fix this? We're trying to deprecate HLRS from https://github.com/opensearch-project/opensearch-java.

@kotwanikunal kotwanikunal added the Clients Clients within the Core repository such as High level Rest client and low level client label Sep 19, 2023
@dennisoelkers
Copy link
Author

Hey @dblock,

not right now. We will switch over to the new client at some point, but it will take significant effort, so we need a short-term solution for the deadlock first.

@msfroh
Copy link
Collaborator

msfroh commented Sep 22, 2023

I have a scrappy fix that I can post a PR for.

It's essentially:

  1. Settings.EMPTY = new Settings(Collections.emptyMap(), null); -- so loading the Settings class doesn't depend on loading Settings.Builder.
  2. Settings.Builder.EMPTY_SETTINGS = Settings.EMPTY -- so we do have a dependency from Settings.Builder to Settings, but that's okay.

@reta reta added v2.11.0 Issues and PRs related to version 2.11.0 v3.0.0 Issues and PRs related to version 3.0.0 labels Oct 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Clients Clients within the Core repository such as High level Rest client and low level client v2.11.0 Issues and PRs related to version 2.11.0 v3.0.0 Issues and PRs related to version 3.0.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants