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

Deadlock in AnalysisSPILoader [LUCENE-10665] #11701

Closed
asfimport opened this issue Jul 28, 2022 · 5 comments · Fixed by #11718
Closed

Deadlock in AnalysisSPILoader [LUCENE-10665] #11701

asfimport opened this issue Jul 28, 2022 · 5 comments · Fixed by #11718

Comments

@asfimport
Copy link

asfimport commented Jul 28, 2022

Loading different TokenFilter/Tokenizer/CharFilter from different threads is causing deadlock. To reproduce use the below code:

public static void main(String[] args) { 
  Thread thread1 = new Thread(UpperCaseFilterFactory::new);
  Thread thread2 = new Thread(LowerCaseFilterFactory::new);
  thread1.start();
  thread2.start();
}

Took the thread dump, and found that thread1 gets stuck while calling ensureClassInitialized(LowerCaseFilterFactory) possibly because thread2 is holding some lock on the same class.


Migrated from LUCENE-10665 by Jasir KT

@uschindler uschindler self-assigned this Aug 24, 2022
@uschindler
Copy link
Contributor

Hi,
this is exactly the same issue we have seen with Codecs and PostingsFormats. The problem is the following: The base class of all analysis factories (TokenizerFactory, TokenFilterFactory, and CharFilterFactory) has a static initializer that runs the SPI lookup. This causes all subclasses to be initialized. If another thread is doing the same it happens that the deadlock occurs.

A workaround was implemented for Codecs and other components in Lucene core, but not yet for analysis components:
https://github.com/apache/lucene/blob/main/lucene/core/src/java/org/apache/lucene/codecs/Codec.java#L40-L59

This must be done in the three base classes in same way. Actually as there is no "default analyzer" the extra checks can be ommitted. It is just important to move the static field into a separate class outside of analysis class hierarchy.

I will tkae care of this issue. In which verison did you see this? We may backport it to 8.x series but I would fix the issue for versions 9.x first.

@uschindler uschindler added this to the 9.4.0 milestone Aug 24, 2022
@uschindler
Copy link
Contributor

See #7541 and #10700 for more details.

@uschindler
Copy link
Contributor

PR (main branch): #11718

@mocobeta mocobeta removed this from the 9.4.0 milestone Aug 25, 2022
uschindler added a commit that referenced this issue Aug 25, 2022
asfgit pushed a commit that referenced this issue Aug 25, 2022
@mocobeta mocobeta added this to the 9.4.0 milestone Aug 27, 2022
@jasirkt
Copy link
Contributor

jasirkt commented Jan 2, 2023

In which verison did you see this?

9.1.0

Thanks for fixing. It works now!

@uschindler
Copy link
Contributor

Thanks for feedback, well appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment