SOLR-12155: awake threads awaiting UIF.<init> despite of exception.

solr/CHANGES.txt

@@ -126,6 +126,8 @@ Bug Fixes
 * SOLR-12207: Just rethrowing AssertionError caused by jdk bug in reflection with invocation details.
  (ab, Dawid Weiss, Mikhail Khludnev)
 
+* SOLR-12155: Exception from UnInvertedField constructor puts threads to infinite wait. (Mikhail Khludnev)
+ 
 Optimizations
 ----------------------
 

solr/core/src/java/org/apache/solr/search/facet/UnInvertedField.java

@@ -35,6 +35,7 @@ import org.apache.lucene.util.BytesRef;
 import org.apache.lucene.util.CharsRefBuilder;
 import org.apache.lucene.util.FixedBitSet;
 import org.apache.solr.common.SolrException;
+import org.apache.solr.common.SolrException.ErrorCode;
 import org.apache.solr.index.SlowCompositeReaderWrapper;
 import org.apache.solr.schema.FieldType;
 import org.apache.solr.schema.TrieField;
@@ -44,6 +45,7 @@ import org.apache.solr.search.DocSet;
 import org.apache.solr.search.SolrCache;
 import org.apache.solr.search.SolrIndexSearcher;
 import org.apache.solr.uninverting.DocTermOrds;
+import org.apache.solr.util.TestInjection;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -106,7 +108,7 @@ public class UnInvertedField extends DocTermOrds {
   private SolrIndexSearcher.DocsEnumState deState;
   private final SolrIndexSearcher searcher;
 
-  private static UnInvertedField uifPlaceholder = new UnInvertedField();
+  private static final UnInvertedField uifPlaceholder = new UnInvertedField();
 
   private UnInvertedField() { // Dummy for synchronization.
     super("fake", 0, 0); // cheapest initialization I can find.
@@ -186,6 +188,8 @@ public class UnInvertedField extends DocTermOrds {
         searcher.maxDoc()/20 + 2,
         DEFAULT_INDEX_INTERVAL_BITS);
 
+    assert TestInjection.injectUIFOutOfMemoryError();
+
     final String prefix = TrieField.getMainValuePrefix(searcher.getSchema().getFieldType(field));
     this.searcher = searcher;
     try {
@@ -556,16 +560,17 @@ public class UnInvertedField extends DocTermOrds {
   //////////////////////////// caching /////////////////////////////
   //////////////////////////////////////////////////////////////////
 
+  @SuppressWarnings("unchecked")
   public static UnInvertedField getUnInvertedField(String field, SolrIndexSearcher searcher) throws IOException {
-    SolrCache<String,UnInvertedField> cache = searcher.getFieldValueCache();
+    SolrCache cache = searcher.getFieldValueCache();
     if (cache == null) {
       return new UnInvertedField(field, searcher);
     }
-    UnInvertedField uif = null;
+
     Boolean doWait = false;
     synchronized (cache) {
-      uif = cache.get(field);
+      final Object val = cache.get(field);
-      if (uif == null) {
+      if (val == null || (val instanceof Throwable)) {
         /**
          * We use this place holder object to pull the UninvertedField construction out of the sync
          * so that if many fields are accessed in a short time, the UninvertedField can be
@@ -573,8 +578,8 @@ public class UnInvertedField extends DocTermOrds {
          */
         cache.put(field, uifPlaceholder);
       } else {
-        if (uif != uifPlaceholder) {
+        if (val != uifPlaceholder) {
-          return uif;
+          return (UnInvertedField) val;
         }
         doWait = true; // Someone else has put the place holder in, wait for that to complete.
       }
@@ -582,35 +587,53 @@ public class UnInvertedField extends DocTermOrds {
     while (doWait) {
       try {
         synchronized (cache) {
-          uif = cache.get(field); // Should at least return the placeholder, NPE if not is OK.
+          final Object val = cache.get(field);
-          if (uif != uifPlaceholder) { // OK, another thread put this in the cache we should be good.
+          if (val != uifPlaceholder) { // OK, another thread put this in the cache we should be good.
-            return uif;
+            if (val instanceof Throwable) {
+              rethrowAsSolrException(field, (Throwable) val);
+            } else {
+              return (UnInvertedField) val;
+            }
           }
           cache.wait();
         }
       } catch (InterruptedException e) {
-        throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "Thread interrupted in getUninvertedField.");
+        rethrowAsSolrException(field, e);
       }
     }
 
-    uif = new UnInvertedField(field, searcher);
+    UnInvertedField uif = null;
+    try {
+      uif = new UnInvertedField(field, searcher);
+    }catch(Throwable e) {
+      synchronized (cache) {
+        cache.put(field, e); // signaling the failure
+        cache.notifyAll();
+      }
+      rethrowAsSolrException(field, e);
+    }
     synchronized (cache) {
       cache.put(field, uif); // Note, this cleverly replaces the placeholder.
       cache.notifyAll();
     }
-
     return uif;
   }
 
+  protected static void rethrowAsSolrException(String field, Throwable e) {
+    throw new SolrException(ErrorCode.SERVER_ERROR, 
+            "Exception occurs during uninverting "+field, e);
+  }
+
   // Returns null if not already populated
+  @SuppressWarnings({"rawtypes", "unchecked"})
   public static UnInvertedField checkUnInvertedField(String field, SolrIndexSearcher searcher) throws IOException {
-    SolrCache<String, UnInvertedField> cache = searcher.getFieldValueCache();
+    SolrCache cache = searcher.getFieldValueCache();
     if (cache == null) {
       return null;
     }
-    UnInvertedField uif = cache.get(field);  // cache is already synchronized, so no extra sync needed
+    Object uif = cache.get(field);  // cache is already synchronized, so no extra sync needed
     // placeholder is an implementation detail, keep it hidden and return null if that is what we got
-    return uif==uifPlaceholder ? null : uif;
+    return uif==uifPlaceholder || !(uif instanceof UnInvertedField)? null : (UnInvertedField) uif;
   }
 
 }

solr/core/src/java/org/apache/solr/util/TestInjection.java

@@ -146,6 +146,8 @@ public class TestInjection {
 
   public static Integer delayBeforeSlaveCommitRefresh=null;
 
+  public static boolean uifOutOfMemoryError = false;
+
   public static void reset() {
     nonGracefullClose = null;
     failReplicaRequests = null;
@@ -161,6 +163,7 @@ public class TestInjection {
     failIndexFingerprintRequests = null;
     wrongIndexFingerprint = null;
     delayBeforeSlaveCommitRefresh = null;
+    uifOutOfMemoryError = false;
 
     for (Timer timer : timers) {
       timer.cancel();
@@ -470,4 +473,11 @@ public class TestInjection {
     return true;
   }
 
+  public static boolean injectUIFOutOfMemoryError() {
+    if (uifOutOfMemoryError ) {
+      throw new OutOfMemoryError("Test Injection");
+    }
+    return true;
+  }
+
 }

solr/core/src/test/org/apache/solr/request/TestUnInvertedFieldException.java

@@ -0,0 +1,127 @@
+/*
+ * 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.solr.request;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Locale;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.Future;
+import java.util.concurrent.LinkedBlockingQueue;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+
+import org.apache.lucene.index.Term;
+import org.apache.lucene.util.NamedThreadFactory;
+import org.apache.lucene.util.TestUtil;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.SolrException.ErrorCode;
+import org.apache.solr.common.util.ExecutorUtil.MDCAwareThreadPoolExecutor;
+import org.apache.solr.search.facet.UnInvertedField;
+import org.apache.solr.util.TestInjection;
+import org.junit.After;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+public class TestUnInvertedFieldException extends SolrTestCaseJ4 {
+  @BeforeClass
+  public static void beforeClass() throws Exception {
+    initCore("solrconfig.xml","schema11.xml");
+  }
+
+  private int numTerms;
+
+  @Override
+  public void setUp() throws Exception {
+    super.setUp();
+    numTerms = TestUtil.nextInt(random(), 10, 50);
+    createIndex(numTerms);
+  }
+
+  @After
+  @Override
+  public void tearDown() throws Exception {
+    super.tearDown();
+  }
+
+  String t(int tnum) {
+    return String.format(Locale.ROOT, "%08d", tnum);
+  }
+  
+  void createIndex(int nTerms) {
+    assertU(delQ("*:*"));
+    for (int i=0; i<nTerms; i++) {
+      assertU(adoc("id", Integer.toString(i), proto.field(), t(i) ));
+    }
+    assertU(commit()); 
+  }
+
+  Term proto = new Term("field_s","");
+
+  @Test
+  public void testConcurrentInit() throws Exception {
+    final SolrQueryRequest req = req("*:*");
+
+    List<Callable<UnInvertedField>> initCallables = new ArrayList<>();
+    for (int i=0;i< TestUtil.nextInt(random(), 10, 30);i++) {
+      initCallables.add(()-> UnInvertedField.getUnInvertedField(proto.field(), req.getSearcher()));
+    }
+
+    final ThreadPoolExecutor pool  = new MDCAwareThreadPoolExecutor(3, 
+        TestUtil.nextInt(random(), 3, 6), 10, TimeUnit.MILLISECONDS,
+        new LinkedBlockingQueue<Runnable>(), new NamedThreadFactory(getClass().getSimpleName()));
+
+    try {
+      TestInjection.uifOutOfMemoryError = true;
+      if (assertsAreEnabled) { // if they aren't, we check that injection is disabled in live
+        List<Future<UnInvertedField>> futures = initCallables.stream().map((c) -> pool.submit(c))
+            .collect(Collectors.toList());
+        for (Future<UnInvertedField> uifuture : futures) {
+          try {
+            final UnInvertedField uif = uifuture.get();
+          } catch (ExecutionException injection) {
+            SolrException solrException = (SolrException) injection.getCause();
+            assertEquals(ErrorCode.SERVER_ERROR.code, solrException.code());
+            assertSame(solrException.getCause().getClass(), OutOfMemoryError.class);
+          }
+          assertNull(UnInvertedField.checkUnInvertedField(proto.field(), req.getSearcher()));
+        }
+        TestInjection.uifOutOfMemoryError = false;
+      }
+      UnInvertedField prev = null;
+      List<Future<UnInvertedField>> futures = initCallables.stream().map((c) -> pool.submit(c))
+          .collect(Collectors.toList());
+      for (Future<UnInvertedField> uifuture : futures) {
+        final UnInvertedField uif = uifuture.get();
+        assertNotNull(uif);
+        assertSame(uif, UnInvertedField.checkUnInvertedField(proto.field(), req.getSearcher()));
+        if (prev != null) {
+          assertSame(prev, uif);
+        }
+        assertEquals(numTerms, uif.numTerms());
+        prev = uif;
+      }
+    } finally {
+      pool.shutdownNow();
+      req.close();
+    }
+  }
+}
+