You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@solr.apache.org by md...@apache.org on 2021/07/21 18:08:17 UTC
[solr] branch main updated: SOLR-15550 Improvements to
ObjectReleaseTracker (#227)
This is an automated email from the ASF dual-hosted git repository.
mdrob pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/solr.git
The following commit(s) were added to refs/heads/main by this push:
new 44e866c SOLR-15550 Improvements to ObjectReleaseTracker (#227)
44e866c is described below
commit 44e866cbe04a19f56b785910e78fcfdbf13d0099
Author: Mike Drob <md...@apache.org>
AuthorDate: Wed Jul 21 11:08:11 2021 -0700
SOLR-15550 Improvements to ObjectReleaseTracker (#227)
* Save submitter stack trace when tracking asynchronously allocated objects
* Defer substantiating exception trace string until we are sure we need it
* Refactor test for increased clarity
---
.../org/apache/solr/cloud/ZkCollectionTerms.java | 4 +-
.../java/org/apache/solr/cloud/ZkShardTerms.java | 4 +-
.../apache/solr/request/SolrQueryRequestBase.java | 7 +
.../solr/common/util/TestObjectReleaseTracker.java | 144 +++++++++++++++++++++
.../apache/solr/util/TestObjectReleaseTracker.java | 61 ---------
.../org/apache/solr/common/util/ExecutorUtil.java | 15 ++-
.../solr/common/util/ObjectReleaseTracker.java | 80 ++++++------
7 files changed, 208 insertions(+), 107 deletions(-)
diff --git a/solr/core/src/java/org/apache/solr/cloud/ZkCollectionTerms.java b/solr/core/src/java/org/apache/solr/cloud/ZkCollectionTerms.java
index 671bb469..f537a54 100644
--- a/solr/core/src/java/org/apache/solr/cloud/ZkCollectionTerms.java
+++ b/solr/core/src/java/org/apache/solr/cloud/ZkCollectionTerms.java
@@ -36,7 +36,7 @@ class ZkCollectionTerms implements AutoCloseable {
this.collection = collection;
this.terms = new HashMap<>();
this.zkClient = client;
- ObjectReleaseTracker.track(this);
+ assert ObjectReleaseTracker.track(this);
}
@@ -65,7 +65,7 @@ class ZkCollectionTerms implements AutoCloseable {
synchronized (terms) {
terms.values().forEach(ZkShardTerms::close);
}
- ObjectReleaseTracker.release(this);
+ assert ObjectReleaseTracker.release(this);
}
}
diff --git a/solr/core/src/java/org/apache/solr/cloud/ZkShardTerms.java b/solr/core/src/java/org/apache/solr/cloud/ZkShardTerms.java
index 6b4799f..160d659 100644
--- a/solr/core/src/java/org/apache/solr/cloud/ZkShardTerms.java
+++ b/solr/core/src/java/org/apache/solr/cloud/ZkShardTerms.java
@@ -103,7 +103,7 @@ public class ZkShardTerms implements AutoCloseable{
ensureTermNodeExist();
refreshTerms();
retryRegisterWatcher();
- ObjectReleaseTracker.track(this);
+ assert ObjectReleaseTracker.track(this);
}
/**
@@ -154,7 +154,7 @@ public class ZkShardTerms implements AutoCloseable{
synchronized (listeners) {
listeners.clear();
}
- ObjectReleaseTracker.release(this);
+ assert ObjectReleaseTracker.release(this);
}
// package private for testing, only used by tests
diff --git a/solr/core/src/java/org/apache/solr/request/SolrQueryRequestBase.java b/solr/core/src/java/org/apache/solr/request/SolrQueryRequestBase.java
index 0d86906..accb225 100644
--- a/solr/core/src/java/org/apache/solr/request/SolrQueryRequestBase.java
+++ b/solr/core/src/java/org/apache/solr/request/SolrQueryRequestBase.java
@@ -33,6 +33,7 @@ import org.apache.solr.common.params.SolrParams;
import org.apache.solr.common.util.CommandOperation;
import org.apache.solr.common.util.ContentStream;
import org.apache.solr.common.util.JsonSchemaValidator;
+import org.apache.solr.common.util.ObjectReleaseTracker;
import org.apache.solr.common.util.SuppressForbidden;
import org.apache.solr.common.util.ValidatingJsonMap;
import org.apache.solr.core.SolrCore;
@@ -126,6 +127,11 @@ public abstract class SolrQueryRequestBase implements SolrQueryRequest, Closeabl
if (searcherHolder==null) {
searcherHolder = core.getSearcher();
+
+ // We start tracking here instead of at construction, because if getSearcher is never called, it's
+ // not fatal to forget close(), and lots of test code is sloppy about it. However, when we get another
+ // searcher reference, having this tracked may be a good hint about where the leak comes from.
+ assert ObjectReleaseTracker.track(this);
}
return searcherHolder.get();
@@ -183,6 +189,7 @@ public abstract class SolrQueryRequestBase implements SolrQueryRequest, Closeabl
@Override
public void close() {
if (searcherHolder!=null) {
+ assert ObjectReleaseTracker.release(this);
searcherHolder.decref();
searcherHolder = null;
}
diff --git a/solr/core/src/test/org/apache/solr/common/util/TestObjectReleaseTracker.java b/solr/core/src/test/org/apache/solr/common/util/TestObjectReleaseTracker.java
new file mode 100644
index 0000000..692b933
--- /dev/null
+++ b/solr/core/src/test/org/apache/solr/common/util/TestObjectReleaseTracker.java
@@ -0,0 +1,144 @@
+/*
+ * 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.common.util;
+
+import org.apache.solr.SolrTestCaseJ4;
+import org.hamcrest.MatcherAssert;
+import org.junit.Test;
+
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicReference;
+
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.stringContainsInOrder;
+
+public class TestObjectReleaseTracker extends SolrTestCaseJ4 {
+
+ @Test
+ public void testReleaseObject() {
+ Object obj = new Object();
+ ObjectReleaseTracker.track(obj);
+ ObjectReleaseTracker.release(obj);
+ assertNull(SolrTestCaseJ4.clearObjectTrackerAndCheckEmpty(1));
+
+ Object obj1 = new Object();
+ ObjectReleaseTracker.track(obj1);
+ Object obj2 = new Object();
+ ObjectReleaseTracker.track(obj2);
+ Object obj3 = new Object();
+ ObjectReleaseTracker.track(obj3);
+
+ ObjectReleaseTracker.release(obj1);
+ ObjectReleaseTracker.release(obj2);
+ ObjectReleaseTracker.release(obj3);
+ assertNull(SolrTestCaseJ4.clearObjectTrackerAndCheckEmpty(1));
+ }
+
+ @Test
+ public void testUnreleased() {
+ Object obj1 = new Object();
+ Object obj2 = new Object();
+ Object obj3 = new Object();
+
+ ObjectReleaseTracker.track(obj1);
+ ObjectReleaseTracker.track(obj2);
+ ObjectReleaseTracker.track(obj3);
+
+ ObjectReleaseTracker.release(obj1);
+ ObjectReleaseTracker.release(obj2);
+ // ObjectReleaseTracker.release(obj3);
+
+ assertNotNull(SolrTestCaseJ4.clearObjectTrackerAndCheckEmpty(1));
+ assertNull(SolrTestCaseJ4.clearObjectTrackerAndCheckEmpty(1));
+ }
+
+ @Test
+ public void testReleaseDifferentObject() {
+ ObjectReleaseTracker.track(new Object());
+ ObjectReleaseTracker.release(new Object());
+ assertNotNull(SolrTestCaseJ4.clearObjectTrackerAndCheckEmpty(1));
+ assertNull(SolrTestCaseJ4.clearObjectTrackerAndCheckEmpty(1));
+ }
+
+ @Test
+ public void testAnonymousClasses() {
+ ObjectReleaseTracker.track(new Object() {});
+ String message = SolrTestCaseJ4.clearObjectTrackerAndCheckEmpty(1);
+ MatcherAssert.assertThat(message, containsString("[Object]"));
+ }
+
+ @Test
+ public void testAsyncTracking() throws InterruptedException, ExecutionException {
+ ExecutorService es = ExecutorUtil.newMDCAwareSingleThreadExecutor(new SolrNamedThreadFactory("TestExec"));
+ Object trackable = new Object();
+
+ Future<?> result = es.submit(() -> {
+ ObjectReleaseTracker.track(trackable);
+ });
+
+ result.get(); // make sure that track has been called
+ String message = SolrTestCaseJ4.clearObjectTrackerAndCheckEmpty(1);
+ MatcherAssert.assertThat(message, stringContainsInOrder(
+ ObjectReleaseTracker.ObjectTrackerException.class.getName(),
+ "Exception: Submitter stack trace",
+ getClassName() + "." + getTestName()));
+
+ // Test the grandparent submitter case
+ AtomicReference<Future<?>> indirectResult = new AtomicReference<>();
+ result = es.submit(() ->
+ indirectResult.set(es.submit(() -> {
+ ObjectReleaseTracker.track(trackable);
+ }))
+ );
+
+ result.get();
+ indirectResult.get().get();
+ message = SolrTestCaseJ4.clearObjectTrackerAndCheckEmpty(1);
+ MatcherAssert.assertThat(message, stringContainsInOrder(
+ ObjectReleaseTracker.ObjectTrackerException.class.getName(),
+ "Exception: Submitter stack trace",
+ "Exception: Submitter stack trace",
+ getClassName() + "." + getTestName()));
+
+ // Now test great-grandparent, which we don't explicitly account for, but should have been recursively set
+ AtomicReference<Future<?>> indirectIndirect = new AtomicReference<>();
+ result = es.submit(() ->
+ indirectResult.set(es.submit(() ->
+ indirectIndirect.set(es.submit(() -> {
+ ObjectReleaseTracker.track(trackable);
+ }))
+ ))
+ );
+
+ result.get();
+ indirectResult.get().get();
+ indirectIndirect.get().get();
+ message = SolrTestCaseJ4.clearObjectTrackerAndCheckEmpty(1);
+ MatcherAssert.assertThat(message, stringContainsInOrder(
+ ObjectReleaseTracker.ObjectTrackerException.class.getName(),
+ "Exception: Submitter stack trace",
+ "Exception: Submitter stack trace",
+ "Exception: Submitter stack trace",
+ getClassName() + "." + getTestName()));
+
+ es.shutdown();
+ assertTrue(es.awaitTermination(1, TimeUnit.SECONDS));
+ }
+}
diff --git a/solr/core/src/test/org/apache/solr/util/TestObjectReleaseTracker.java b/solr/core/src/test/org/apache/solr/util/TestObjectReleaseTracker.java
deleted file mode 100644
index 312c99b..0000000
--- a/solr/core/src/test/org/apache/solr/util/TestObjectReleaseTracker.java
+++ /dev/null
@@ -1,61 +0,0 @@
-/*
- * 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.util;
-
-import org.apache.lucene.util.TestRuleLimitSysouts.Limit;
-import org.apache.solr.SolrTestCaseJ4;
-import org.apache.solr.common.util.ObjectReleaseTracker;
-import org.junit.Test;
-
-
-@Limit(bytes=150000) // raise limit as this writes to sys err
-public class TestObjectReleaseTracker extends SolrTestCaseJ4 {
-
- @Test
- public void testObjectReleaseTracker() {
- ObjectReleaseTracker.track(new Object());
- ObjectReleaseTracker.release(new Object());
- assertNotNull(SolrTestCaseJ4.clearObjectTrackerAndCheckEmpty(1));
- assertNull(SolrTestCaseJ4.clearObjectTrackerAndCheckEmpty(1));
- Object obj = new Object();
- ObjectReleaseTracker.track(obj);
- ObjectReleaseTracker.release(obj);
- assertNull(SolrTestCaseJ4.clearObjectTrackerAndCheckEmpty(1));
-
- Object obj1 = new Object();
- ObjectReleaseTracker.track(obj1);
- Object obj2 = new Object();
- ObjectReleaseTracker.track(obj2);
- Object obj3 = new Object();
- ObjectReleaseTracker.track(obj3);
-
- ObjectReleaseTracker.release(obj1);
- ObjectReleaseTracker.release(obj2);
- ObjectReleaseTracker.release(obj3);
- assertNull(SolrTestCaseJ4.clearObjectTrackerAndCheckEmpty(1));
-
- ObjectReleaseTracker.track(obj1);
- ObjectReleaseTracker.track(obj2);
- ObjectReleaseTracker.track(obj3);
-
- ObjectReleaseTracker.release(obj1);
- ObjectReleaseTracker.release(obj2);
- // ObjectReleaseTracker.release(obj3);
- assertNotNull(SolrTestCaseJ4.clearObjectTrackerAndCheckEmpty(1));
- assertNull(SolrTestCaseJ4.clearObjectTrackerAndCheckEmpty(1));
- }
-}
diff --git a/solr/solrj/src/java/org/apache/solr/common/util/ExecutorUtil.java b/solr/solrj/src/java/org/apache/solr/common/util/ExecutorUtil.java
index dadb690..47838eb 100644
--- a/solr/solrj/src/java/org/apache/solr/common/util/ExecutorUtil.java
+++ b/solr/solrj/src/java/org/apache/solr/common/util/ExecutorUtil.java
@@ -39,6 +39,8 @@ import org.slf4j.MDC;
public class ExecutorUtil {
private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+ static final ThreadLocal<Throwable> submitter = new ThreadLocal<>();
+
private static volatile List<InheritableThreadLocalProvider> providers = new ArrayList<>();
/**
@@ -110,7 +112,7 @@ public class ExecutorUtil {
public static ExecutorService newMDCAwareFixedThreadPool(int nThreads, ThreadFactory threadFactory) {
return new MDCAwareThreadPoolExecutor(nThreads, nThreads,
0L, TimeUnit.MILLISECONDS,
- new LinkedBlockingQueue<Runnable>(),
+ new LinkedBlockingQueue<>(),
threadFactory);
}
@@ -196,7 +198,13 @@ public class ExecutorUtil {
String ctxStr = contextString.toString().replace("/", "//");
final String submitterContextStr = ctxStr.length() <= MAX_THREAD_NAME_LEN ? ctxStr : ctxStr.substring(0, MAX_THREAD_NAME_LEN);
- final Exception submitterStackTrace = enableSubmitterStackTrace ? new Exception("Submitter stack trace") : null;
+ final Throwable submitterStackTrace; // Never thrown, only used as stack trace holder
+ if (enableSubmitterStackTrace) {
+ Throwable grandParentSubmitter = submitter.get();
+ submitterStackTrace = new Exception("Submitter stack trace", grandParentSubmitter);
+ } else {
+ submitterStackTrace = null;
+ }
final List<InheritableThreadLocalProvider> providersCopy = providers;
final ArrayList<AtomicReference<Object>> ctx = providersCopy.isEmpty() ? null : new ArrayList<>(providersCopy.size());
if (ctx != null) {
@@ -220,6 +228,9 @@ public class ExecutorUtil {
} else {
MDC.clear();
}
+ if (enableSubmitterStackTrace) {
+ submitter.set(submitterStackTrace);
+ }
try {
command.run();
} catch (Throwable t) {
diff --git a/solr/solrj/src/java/org/apache/solr/common/util/ObjectReleaseTracker.java b/solr/solrj/src/java/org/apache/solr/common/util/ObjectReleaseTracker.java
index bbd666a..59927b1 100644
--- a/solr/solrj/src/java/org/apache/solr/common/util/ObjectReleaseTracker.java
+++ b/solr/solrj/src/java/org/apache/solr/common/util/ObjectReleaseTracker.java
@@ -22,10 +22,8 @@ import java.io.PrintWriter;
import java.io.StringWriter;
import java.lang.invoke.MethodHandles;
import java.util.ArrayList;
-import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
-import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ExecutorService;
@@ -34,13 +32,12 @@ import org.slf4j.LoggerFactory;
public class ObjectReleaseTracker {
private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
- public static Map<Object,String> OBJECTS = new ConcurrentHashMap<>();
+ public static final Map<Object, Exception> OBJECTS = new ConcurrentHashMap<>();
public static boolean track(Object object) {
- StringWriter sw = new StringWriter();
- PrintWriter pw = new PrintWriter(sw);
- new ObjectTrackerException(object.getClass().getName()).printStackTrace(pw);
- OBJECTS.put(object, sw.toString());
+ // This is called from within constructors, be careful not to make assumptions about state of object here
+ Throwable submitter = ExecutorUtil.submitter.get(); // Could be null
+ OBJECTS.put(object, new ObjectTrackerException(object.getClass().getName(), submitter));
return true;
}
@@ -57,50 +54,53 @@ public class ObjectReleaseTracker {
* @return null if ok else error message
*/
public static String checkEmpty() {
- String error = null;
- Set<Entry<Object,String>> entries = OBJECTS.entrySet();
+ if (OBJECTS.isEmpty()) {
+ return null;
+ }
- if (entries.size() > 0) {
- List<String> objects = new ArrayList<>();
- for (Entry<Object,String> entry : entries) {
- objects.add(entry.getKey().getClass().getSimpleName());
- }
-
- error = "ObjectTracker found " + entries.size() + " object(s) that were not released!!! " + objects + "\n";
- for (Entry<Object,String> entry : entries) {
- error += entry.getValue() + "\n";
- }
+ StringBuilder error = new StringBuilder();
+ error.append("ObjectTracker found ").append(OBJECTS.size()).append(" object(s) that were not released!!! ");
+
+ ArrayList<String> objects = new ArrayList<>(OBJECTS.size());
+ for (Object object : OBJECTS.keySet()) {
+ Class<?> clazz = object.getClass();
+ objects.add(clazz.isAnonymousClass() ? clazz.getSuperclass().getSimpleName() : clazz.getSimpleName());
+ }
+ error.append(objects).append("\n");
+
+ for (Entry<Object, Exception> entry : OBJECTS.entrySet()) {
+ StringWriter sw = new StringWriter();
+ PrintWriter pw = new PrintWriter(sw);
+ entry.getValue().printStackTrace(pw);
+
+ error.append(entry.getKey().getClass().getName()).append(":");
+ error.append(sw).append("\n");
}
- return error;
+ return error.toString();
}
public static void tryClose() {
- Set<Entry<Object,String>> entries = OBJECTS.entrySet();
-
- if (entries.size() > 0) {
- for (Entry<Object,String> entry : entries) {
- if (entry.getKey() instanceof Closeable) {
- try {
- ((Closeable)entry.getKey()).close();
- } catch (Throwable t) {
- log.error("", t);
- }
- } else if (entry.getKey() instanceof ExecutorService) {
- try {
- ExecutorUtil.shutdownAndAwaitTermination((ExecutorService)entry.getKey());
- } catch (Throwable t) {
- log.error("", t);
- }
+ for (Object object : OBJECTS.keySet()) {
+ if (object instanceof Closeable) {
+ try {
+ ((Closeable) object).close();
+ } catch (Throwable t) {
+ log.error("", t);
+ }
+ } else if (object instanceof ExecutorService) {
+ try {
+ ExecutorUtil.shutdownAndAwaitTermination((ExecutorService) object);
+ } catch (Throwable t) {
+ log.error("", t);
}
}
}
}
- private static class ObjectTrackerException extends RuntimeException {
- ObjectTrackerException(String msg) {
- super(msg);
+ static class ObjectTrackerException extends RuntimeException {
+ ObjectTrackerException(String msg, Throwable submitter) {
+ super(msg, submitter);
}
}
-
}