You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2021/01/13 15:31:27 UTC
[GitHub] [lucene-solr] danmuzi opened a new pull request #2200: LUCENE-9661: Fix deadlock in TermsEnum.EMPTY
danmuzi opened a new pull request #2200:
URL: https://github.com/apache/lucene-solr/pull/2200
Deadlocks can occur if the constructor part of BaseTermsEnum is executed during initializing of TermsEnum.
It can be reproduced by revert the code of TermsEnum class and running TestTermsEnumDeadlock#testDeadlock.
This PR will be cherry-picked to the master, 8x and 8.8 branch.
Issue link: https://issues.apache.org/jira/browse/LUCENE-9661
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] danmuzi commented on pull request #2200: LUCENE-9661: Fix deadlock in TermsEnum.EMPTY
Posted by GitBox <gi...@apache.org>.
danmuzi commented on pull request #2200:
URL: https://github.com/apache/lucene-solr/pull/2200#issuecomment-760293515
If the precommit and musedev builds are passed and comment is not posted until 2021-01-15 19:30 (UTC),
I'll submit this PR to master branch for release.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] madrob commented on a change in pull request #2200: LUCENE-9661: Fix deadlock in TermsEnum.EMPTY
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #2200:
URL: https://github.com/apache/lucene-solr/pull/2200#discussion_r556614634
##########
File path: lucene/core/src/test/org/apache/lucene/index/TestTermsEnumDeadlock.java
##########
@@ -0,0 +1,133 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.index;
+
+import org.apache.lucene.util.BytesRef;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.io.IOException;
+import java.lang.management.ManagementFactory;
+import java.lang.management.RuntimeMXBean;
+import java.nio.file.Paths;
+import java.util.concurrent.ThreadLocalRandom;
+import java.util.concurrent.TimeUnit;
+
+public class TestTermsEnumDeadlock extends Assert {
+ private static final int MAX_TIME_SECONDS = 15;
+
+ @Test
+ public void testDeadlock() throws Exception {
+ for (int i = 0; i < 20; i++) {
+ // Fork a separate JVM to reinitialize classes.
Review comment:
Could we do this by creating a new class loader instead of a whole new process?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] danmuzi commented on pull request #2200: LUCENE-9661: Fix deadlock in TermsEnum.EMPTY
Posted by GitBox <gi...@apache.org>.
danmuzi commented on pull request #2200:
URL: https://github.com/apache/lucene-solr/pull/2200#issuecomment-761214805
Thank you for your reviews, @mikemccand and @uschindler!
I checked your comments and opened a new JIRA issue for this problem.
(https://issues.apache.org/jira/browse/LUCENE-9672)
I'd appreciate it if you could check that I understood it correctly :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] danmuzi commented on a change in pull request #2200: LUCENE-9661: Fix deadlock in TermsEnum.EMPTY
Posted by GitBox <gi...@apache.org>.
danmuzi commented on a change in pull request #2200:
URL: https://github.com/apache/lucene-solr/pull/2200#discussion_r556689874
##########
File path: lucene/core/src/test/org/apache/lucene/index/TestTermsEnumDeadlock.java
##########
@@ -0,0 +1,133 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.index;
+
+import org.apache.lucene.util.BytesRef;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.io.IOException;
+import java.lang.management.ManagementFactory;
+import java.lang.management.RuntimeMXBean;
+import java.nio.file.Paths;
+import java.util.concurrent.ThreadLocalRandom;
+import java.util.concurrent.TimeUnit;
+
+public class TestTermsEnumDeadlock extends Assert {
+ private static final int MAX_TIME_SECONDS = 15;
+
+ @Test
+ public void testDeadlock() throws Exception {
+ for (int i = 0; i < 20; i++) {
+ // Fork a separate JVM to reinitialize classes.
Review comment:
Thank you for your review @madrob :D
Hmm... Is there any way to do static initialization multiple times in the same JVM?
As far as I know, static initialization is only executed once per JVM and the concepts of loading and initialization are different.
https://docs.oracle.com/javase/specs/jvms/se11/html/jvms-5.html
Class loading : https://docs.oracle.com/javase/specs/jvms/se11/html/jvms-5.html#jvms-5.3
Initialization : https://docs.oracle.com/javase/specs/jvms/se11/html/jvms-5.html#jvms-5.5
And that TC is meaningful only when the test is run the first time or alone.
Please let me know if I'm wrong!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] danmuzi merged pull request #2200: LUCENE-9661: Fix deadlock in TermsEnum.EMPTY
Posted by GitBox <gi...@apache.org>.
danmuzi merged pull request #2200:
URL: https://github.com/apache/lucene-solr/pull/2200
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] danmuzi commented on a change in pull request #2200: LUCENE-9661: Fix deadlock in TermsEnum.EMPTY
Posted by GitBox <gi...@apache.org>.
danmuzi commented on a change in pull request #2200:
URL: https://github.com/apache/lucene-solr/pull/2200#discussion_r556740515
##########
File path: lucene/core/src/test/org/apache/lucene/index/TestTermsEnumDeadlock.java
##########
@@ -0,0 +1,133 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.index;
+
+import org.apache.lucene.util.BytesRef;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.io.IOException;
+import java.lang.management.ManagementFactory;
+import java.lang.management.RuntimeMXBean;
+import java.nio.file.Paths;
+import java.util.concurrent.ThreadLocalRandom;
+import java.util.concurrent.TimeUnit;
+
+public class TestTermsEnumDeadlock extends Assert {
+ private static final int MAX_TIME_SECONDS = 15;
+
+ @Test
+ public void testDeadlock() throws Exception {
+ for (int i = 0; i < 20; i++) {
+ // Fork a separate JVM to reinitialize classes.
Review comment:
Thank you for your review @uschindler :D
It was added to ensure working.
But I agree with your comment about test iteration.
So I'll delete the for-loop statement and change it to run only once.
As for the necessity of deadlock test, I'll add TODO comment.
Later, if someone applies to static checker about it, I'll delete it at that time.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on pull request #2200: LUCENE-9661: Fix deadlock in TermsEnum.EMPTY
Posted by GitBox <gi...@apache.org>.
dsmiley commented on pull request #2200:
URL: https://github.com/apache/lucene-solr/pull/2200#issuecomment-760271205
> What I understand is that it also requires a new JVM for each testing.
Right; what Uwe said. It'd make for a nice nightly CI sort of thing executed via gradle but perhaps not a standard JUnit test. If you're feeling ambitious then go for it but you certainly don't have to for this nice-to-have sort of thing. If I were so inclined, I might direct such energies into error-prone so that not only would we benefit (we use that) but so would others.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] danmuzi commented on pull request #2200: LUCENE-9661: Fix deadlock in TermsEnum.EMPTY
Posted by GitBox <gi...@apache.org>.
danmuzi commented on pull request #2200:
URL: https://github.com/apache/lucene-solr/pull/2200#issuecomment-760262251
Thank you for your review, @dsmiley!
> I don't think we should have a test that checks for this problem on just one class.
Okay. I'll delete the test case.
In fact, I was thinking about it too :)
> some test that scans classes and loads them in random order.
What I understand is that it also requires a new JVM for each testing.
(Let me know if I misunderstood your content!)
I'm not sure there's any other good way.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] uschindler commented on a change in pull request #2200: LUCENE-9661: Fix deadlock in TermsEnum.EMPTY
Posted by GitBox <gi...@apache.org>.
uschindler commented on a change in pull request #2200:
URL: https://github.com/apache/lucene-solr/pull/2200#discussion_r556698348
##########
File path: lucene/core/src/test/org/apache/lucene/index/TestTermsEnumDeadlock.java
##########
@@ -0,0 +1,133 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.index;
+
+import org.apache.lucene.util.BytesRef;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.io.IOException;
+import java.lang.management.ManagementFactory;
+import java.lang.management.RuntimeMXBean;
+import java.nio.file.Paths;
+import java.util.concurrent.ThreadLocalRandom;
+import java.util.concurrent.TimeUnit;
+
+public class TestTermsEnumDeadlock extends Assert {
+ private static final int MAX_TIME_SECONDS = 15;
+
+ @Test
+ public void testDeadlock() throws Exception {
+ for (int i = 0; i < 20; i++) {
+ // Fork a separate JVM to reinitialize classes.
Review comment:
Classloading won't help, because we still need a separate JVM. When JVM loads classes, it trys in parent classloader first. If the class is already there it won't load. So we need at least one separate process.
I don't like to try many times each with separate JVM. Maybe only try once (like in the other test with codecs). It may not fail every time, but sometimes test fails.
I am also not sure if we really need a test for this. If we may get a static checker that finds classes that initialize their subclasses in their own static initializer, we can prevent similar cars in future.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] danmuzi commented on pull request #2200: LUCENE-9661: Fix deadlock in TermsEnum.EMPTY
Posted by GitBox <gi...@apache.org>.
danmuzi commented on pull request #2200:
URL: https://github.com/apache/lucene-solr/pull/2200#issuecomment-760271836
Additional fixes : The attributes method in EMPTY has been changed to the same as before.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] mikemccand commented on pull request #2200: LUCENE-9661: Fix deadlock in TermsEnum.EMPTY
Posted by GitBox <gi...@apache.org>.
mikemccand commented on pull request #2200:
URL: https://github.com/apache/lucene-solr/pull/2200#issuecomment-761015258
> If we may get a static checker that finds classes that initialize their subclasses in their own static initializer, we can prevent similar cars in future.
Let's open a new issue to explore this? A static checker to prevent subtle rare deadlocks seems really really important :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] uschindler commented on pull request #2200: LUCENE-9661: Fix deadlock in TermsEnum.EMPTY
Posted by GitBox <gi...@apache.org>.
uschindler commented on pull request #2200:
URL: https://github.com/apache/lucene-solr/pull/2200#issuecomment-761100335
IntelliJ has a check for this which works quite good. But in contrast ECJ from Eclipse has not. It would have been great to allow this check.
Maybe check errorprone, maybe they have a rule already? Basically we could add this to forbiddenapis, but that would be a misnomer. All you need is a check if a static initializer of a class references a subclass in any (direct) way. The issue then happens if the subclass is initialized in another thread while the static initializer is running. So generally static initializers that refer to private child classes which cannot be seen befor the parent is initialized, are safe, but that's a detail and needs to be decided.
The Codec Deadlock was exactly that problem, too. Unfortunately for this case a static tool won't have a chance, so a static tool alone is not a full solution.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on pull request #2200: LUCENE-9661: Fix deadlock in TermsEnum.EMPTY
Posted by GitBox <gi...@apache.org>.
dsmiley commented on pull request #2200:
URL: https://github.com/apache/lucene-solr/pull/2200#issuecomment-759782886
I don't think we should have a test that checks for this problem on just one class. Either we have no test for this whatsoever (totally fine IMO), or some test that scans classes and loads them in random order.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] uschindler commented on a change in pull request #2200: LUCENE-9661: Fix deadlock in TermsEnum.EMPTY
Posted by GitBox <gi...@apache.org>.
uschindler commented on a change in pull request #2200:
URL: https://github.com/apache/lucene-solr/pull/2200#discussion_r556698348
##########
File path: lucene/core/src/test/org/apache/lucene/index/TestTermsEnumDeadlock.java
##########
@@ -0,0 +1,133 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.index;
+
+import org.apache.lucene.util.BytesRef;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.io.IOException;
+import java.lang.management.ManagementFactory;
+import java.lang.management.RuntimeMXBean;
+import java.nio.file.Paths;
+import java.util.concurrent.ThreadLocalRandom;
+import java.util.concurrent.TimeUnit;
+
+public class TestTermsEnumDeadlock extends Assert {
+ private static final int MAX_TIME_SECONDS = 15;
+
+ @Test
+ public void testDeadlock() throws Exception {
+ for (int i = 0; i < 20; i++) {
+ // Fork a separate JVM to reinitialize classes.
Review comment:
Classloading won't help, because we still need a separate JVM. When JVM loads classes, it ttys in parent classliadee first. If the class is already there it won't load. So we need at least one separate process.
I don't like to try many times each with separate JVM. Maybe only try once (like in the other test with codecs). It may not fail every time, but sometimes test fails.
I am also not sure if we really need a test for this. If we may get a static checker that finds classes that initialize their subclasses in their own static initializer, we can prevent similar cars in future.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] danmuzi commented on pull request #2200: LUCENE-9661: Fix deadlock in TermsEnum.EMPTY
Posted by GitBox <gi...@apache.org>.
danmuzi commented on pull request #2200:
URL: https://github.com/apache/lucene-solr/pull/2200#issuecomment-760281560
Thank you @dsmiley
I'll think about the value of that.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org