You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@solr.apache.org by ho...@apache.org on 2023/09/21 18:57:55 UTC
[solr] branch main updated: SOLR-16983: Fixed a bug that could cause some usages of SolrStream to fail to close InputStreams from the server.
This is an automated email from the ASF dual-hosted git repository.
hossman 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 117a6cdf415 SOLR-16983: Fixed a bug that could cause some usages of SolrStream to fail to close InputStreams from the server.
117a6cdf415 is described below
commit 117a6cdf415ae43e3279aa1ea91adc8180a1cf74
Author: Chris Hostetter <ho...@apache.org>
AuthorDate: Thu Sep 21 11:57:46 2023 -0700
SOLR-16983: Fixed a bug that could cause some usages of SolrStream to fail to close InputStreams from the server.
Also fixed the usage of ObjectReleaseTracker in SolrTestCaseJ4 to catch these kinds of bugs
---
solr/CHANGES.txt | 3 +
.../solr/metrics/SolrMetricsIntegrationTest.java | 79 +++++++++++-----------
.../apache/solr/response/TestRawTransformer.java | 6 +-
.../apache/solr/servlet/HideStackTraceTest.java | 7 +-
.../solr/client/solrj/io/stream/SolrStream.java | 8 +--
.../solr/client/solrj/impl/Http2SolrClient.java | 38 +++++++++--
.../solrj/embedded/SolrExampleJettyTest.java | 5 +-
.../client/solrj/impl/BasicHttpSolrClientTest.java | 6 +-
.../client/solrj/impl/Http2SolrClientTest.java | 42 +++++++-----
.../src/java/org/apache/solr/SolrTestCaseJ4.java | 10 ++-
10 files changed, 131 insertions(+), 73 deletions(-)
diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt
index fd6b9a669bd..ba8adb622de 100644
--- a/solr/CHANGES.txt
+++ b/solr/CHANGES.txt
@@ -198,6 +198,9 @@ Bug Fixes
* SOLR-16931: ReRankScaler explain breaks with debug=true and in distributed mode (Joel Bernstein)
+* SOLR-16983: Fixed a bug that could cause some usages of SolrStream to fail to close InputStreams from the server.
+ Also fixed the usage of ObjectReleaseTracker in SolrTestCaseJ4 to catch these kinds of bugs (hossman)
+
Dependency Upgrades
---------------------
diff --git a/solr/core/src/test/org/apache/solr/metrics/SolrMetricsIntegrationTest.java b/solr/core/src/test/org/apache/solr/metrics/SolrMetricsIntegrationTest.java
index 7f77a701d05..06e9a59f54c 100644
--- a/solr/core/src/test/org/apache/solr/metrics/SolrMetricsIntegrationTest.java
+++ b/solr/core/src/test/org/apache/solr/metrics/SolrMetricsIntegrationTest.java
@@ -31,6 +31,7 @@ import java.util.Set;
import org.apache.http.client.HttpClient;
import org.apache.lucene.tests.util.TestUtil;
import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.client.solrj.SolrClient;
import org.apache.solr.client.solrj.impl.HttpSolrClient;
import org.apache.solr.cloud.MiniSolrCloudCluster;
import org.apache.solr.common.util.Utils;
@@ -233,47 +234,49 @@ public class SolrMetricsIntegrationTest extends SolrTestCaseJ4 {
try {
JettySolrRunner j = cluster.getRandomJetty(random());
String url = j.getBaseUrl() + "/admin/metrics?key=solr.node:CONTAINER.zkClient&wt=json";
- HttpClient httpClient = ((HttpSolrClient) j.newClient()).getHttpClient();
- @SuppressWarnings("unchecked")
- Map<String, Object> zkMmetrics =
- (Map<String, Object>)
- Utils.getObjectByPath(
- Utils.executeGET(httpClient, url, Utils.JSONCONSUMER),
- false,
- List.of("metrics", "solr.node:CONTAINER.zkClient"));
+ try (SolrClient solrClient = j.newClient()) {
+ HttpClient httpClient = ((HttpSolrClient) solrClient).getHttpClient();
+ @SuppressWarnings("unchecked")
+ Map<String, Object> zkMmetrics =
+ (Map<String, Object>)
+ Utils.getObjectByPath(
+ Utils.executeGET(httpClient, url, Utils.JSONCONSUMER),
+ false,
+ List.of("metrics", "solr.node:CONTAINER.zkClient"));
- Set<String> allKeys =
- Set.of(
- "watchesFired",
- "reads",
- "writes",
- "bytesRead",
- "bytesWritten",
- "multiOps",
- "cumulativeMultiOps",
- "childFetches",
- "cumulativeChildrenFetched",
- "existsChecks",
- "deletes");
+ Set<String> allKeys =
+ Set.of(
+ "watchesFired",
+ "reads",
+ "writes",
+ "bytesRead",
+ "bytesWritten",
+ "multiOps",
+ "cumulativeMultiOps",
+ "childFetches",
+ "cumulativeChildrenFetched",
+ "existsChecks",
+ "deletes");
- for (String k : allKeys) {
- assertNotNull(zkMmetrics.get(k));
- }
- Utils.executeGET(
- httpClient,
- j.getBaseURLV2() + "/cluster/zookeeper/children/live_nodes",
- Utils.JSONCONSUMER);
- @SuppressWarnings("unchecked")
- Map<String, Object> zkMmetricsNew =
- (Map<String, Object>)
- Utils.getObjectByPath(
- Utils.executeGET(httpClient, url, Utils.JSONCONSUMER),
- false,
- List.of("metrics", "solr.node:CONTAINER.zkClient"));
+ for (String k : allKeys) {
+ assertNotNull(zkMmetrics.get(k));
+ }
+ Utils.executeGET(
+ httpClient,
+ j.getBaseURLV2() + "/cluster/zookeeper/children/live_nodes",
+ Utils.JSONCONSUMER);
+ @SuppressWarnings("unchecked")
+ Map<String, Object> zkMmetricsNew =
+ (Map<String, Object>)
+ Utils.getObjectByPath(
+ Utils.executeGET(httpClient, url, Utils.JSONCONSUMER),
+ false,
+ List.of("metrics", "solr.node:CONTAINER.zkClient"));
- assertTrue(findDelta(zkMmetrics, zkMmetricsNew, "childFetches") >= 1);
- assertTrue(findDelta(zkMmetrics, zkMmetricsNew, "cumulativeChildrenFetched") >= 3);
- assertTrue(findDelta(zkMmetrics, zkMmetricsNew, "existsChecks") >= 4);
+ assertTrue(findDelta(zkMmetrics, zkMmetricsNew, "childFetches") >= 1);
+ assertTrue(findDelta(zkMmetrics, zkMmetricsNew, "cumulativeChildrenFetched") >= 3);
+ assertTrue(findDelta(zkMmetrics, zkMmetricsNew, "existsChecks") >= 4);
+ }
} finally {
cluster.shutdown();
}
diff --git a/solr/core/src/test/org/apache/solr/response/TestRawTransformer.java b/solr/core/src/test/org/apache/solr/response/TestRawTransformer.java
index 393674c2eca..c68ada54af6 100644
--- a/solr/core/src/test/org/apache/solr/response/TestRawTransformer.java
+++ b/solr/core/src/test/org/apache/solr/response/TestRawTransformer.java
@@ -118,10 +118,14 @@ public class TestRawTransformer extends SolrCloudTestCase {
@AfterClass
public static void afterClass() throws Exception {
+ if (CLIENT != null) {
+ org.apache.solr.common.util.IOUtils.closeQuietly(CLIENT);
+ CLIENT = null;
+ }
if (JSR != null) {
JSR.stop();
+ JSR = null;
}
- // NOTE: CLOUD_CLIENT should be stopped automatically in `SolrCloudTestCase.shutdownCluster()`
}
@After
diff --git a/solr/core/src/test/org/apache/solr/servlet/HideStackTraceTest.java b/solr/core/src/test/org/apache/solr/servlet/HideStackTraceTest.java
index dc6af6f2716..54ac3e28b5a 100644
--- a/solr/core/src/test/org/apache/solr/servlet/HideStackTraceTest.java
+++ b/solr/core/src/test/org/apache/solr/servlet/HideStackTraceTest.java
@@ -145,13 +145,16 @@ public class HideStackTraceTest extends SolrTestCaseJ4 {
final String url = solrRule.getBaseUrl().toString() + "/collection1/withError?q=*:*&wt=json";
final HttpGet get = new HttpGet(url);
- try (var client = HttpClientUtil.createClient(null);
- CloseableHttpResponse response = client.execute(get)) {
+ var client = HttpClientUtil.createClient(null);
+ try (CloseableHttpResponse response = client.execute(get)) {
+
assertEquals(500, response.getStatusLine().getStatusCode());
String responseJson = EntityUtils.toString(response.getEntity());
assertFalse(responseJson.contains("\"trace\""));
assertFalse(
responseJson.contains("org.apache.solr.servlet.HideStackTraceTest$ErrorComponent"));
+ } finally {
+ HttpClientUtil.close(client);
}
}
diff --git a/solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/stream/SolrStream.java b/solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/stream/SolrStream.java
index ce4d31babaa..1f06c852fd1 100644
--- a/solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/stream/SolrStream.java
+++ b/solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/stream/SolrStream.java
@@ -41,6 +41,7 @@ import org.apache.solr.common.params.CommonParams;
import org.apache.solr.common.params.ModifiableSolrParams;
import org.apache.solr.common.params.SolrParams;
import org.apache.solr.common.params.StreamParams;
+import org.apache.solr.common.util.IOUtils;
import org.apache.solr.common.util.NamedList;
/**
@@ -187,11 +188,10 @@ public class SolrStream extends TupleStream {
/** Closes the Stream to a single Solr Instance */
@Override
public void close() throws IOException {
- if (closeableHttpResponse != null) {
- closeableHttpResponse.close();
- }
+ IOUtils.closeQuietly(tupleStreamParser);
+ IOUtils.closeQuietly(closeableHttpResponse);
if (doCloseCache) {
- clientCache.close();
+ IOUtils.closeQuietly(clientCache);
}
}
diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java
index 2d6a993be7c..8dbbe3e62df 100644
--- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java
+++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java
@@ -20,6 +20,7 @@ import static org.apache.solr.common.util.Utils.getObjectByPath;
import java.io.ByteArrayOutputStream;
import java.io.Closeable;
+import java.io.FilterInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.lang.invoke.MethodHandles;
@@ -418,7 +419,7 @@ public class Http2SolrClient extends SolrClient {
.method(HttpMethod.POST)
.body(content);
decorateRequest(postRequest, updateRequest, false);
- InputStreamResponseListener responseListener = new InputStreamResponseListener();
+ InputStreamResponseListener responseListener = new InputStreamReleaseTrackingResponseListener();
postRequest.send(responseListener);
boolean isXml = ClientUtils.TEXT_XML.equals(requestWriter.getUpdateContentType());
@@ -468,14 +469,13 @@ public class Http2SolrClient extends SolrClient {
try {
String url = getRequestPath(solrRequest, collection);
InputStreamResponseListener listener =
- new InputStreamResponseListener() {
+ new InputStreamReleaseTrackingResponseListener() {
@Override
public void onHeaders(Response response) {
super.onHeaders(response);
executor.execute(
() -> {
InputStream is = getInputStream();
- assert ObjectReleaseTracker.track(is);
try {
NamedList<Object> body =
processErrorsAndResponse(solrRequest, response, is, url);
@@ -525,12 +525,11 @@ public class Http2SolrClient extends SolrClient {
Throwable abortCause = null;
Request req = null;
try {
- InputStreamResponseListener listener = new InputStreamResponseListener();
+ InputStreamResponseListener listener = new InputStreamReleaseTrackingResponseListener();
req = makeRequestAndSend(solrRequest, url, listener, false);
Response response = listener.get(idleTimeoutMillis, TimeUnit.MILLISECONDS);
url = req.getURI().toString();
InputStream is = listener.getInputStream();
- assert ObjectReleaseTracker.track(is);
return processErrorsAndResponse(solrRequest, response, is, url);
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
@@ -928,7 +927,6 @@ public class Http2SolrClient extends SolrClient {
if (shouldClose) {
try {
is.close();
- assert ObjectReleaseTracker.release(is);
} catch (IOException e) {
// quitely
}
@@ -1417,4 +1415,32 @@ public class Http2SolrClient extends SolrClient {
}
}
}
+
+ /**
+ * Extension of InputStreamResponseListener that handles Object release tracking of the
+ * InputStreams
+ *
+ * @see ObjectReleaseTracker
+ */
+ private static class InputStreamReleaseTrackingResponseListener
+ extends InputStreamResponseListener {
+
+ @Override
+ public InputStream getInputStream() {
+ return new ObjectReleaseTrackedInputStream(super.getInputStream());
+ }
+
+ private static final class ObjectReleaseTrackedInputStream extends FilterInputStream {
+ public ObjectReleaseTrackedInputStream(final InputStream in) {
+ super(in);
+ assert ObjectReleaseTracker.track(in);
+ }
+
+ @Override
+ public void close() throws IOException {
+ assert ObjectReleaseTracker.release(in);
+ super.close();
+ }
+ }
+ }
}
diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/embedded/SolrExampleJettyTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/embedded/SolrExampleJettyTest.java
index 11a593d82c3..ee9bc159c62 100644
--- a/solr/solrj/src/test/org/apache/solr/client/solrj/embedded/SolrExampleJettyTest.java
+++ b/solr/solrj/src/test/org/apache/solr/client/solrj/embedded/SolrExampleJettyTest.java
@@ -66,6 +66,8 @@ public class SolrExampleJettyTest extends SolrExampleTests {
@Test
public void testArbitraryJsonIndexing() throws Exception {
+ // For making raw requests...
+ HttpClient httpClient = HttpClientUtil.createClient(null);
try (SolrClient client = getSolrClient()) {
client.deleteByQuery("*:*");
client.commit();
@@ -73,7 +75,6 @@ public class SolrExampleJettyTest extends SolrExampleTests {
// two docs, one with uniqueKey, another without it
String json = "{\"id\":\"abc1\", \"name\": \"name1\"} {\"name\" : \"name2\"}";
- HttpClient httpClient = getHttpClient(getCoreUrl());
HttpPost post = new HttpPost(getRandomizedUpdateUri(getCoreUrl()));
post.setHeader("Content-Type", "application/json");
post.setEntity(
@@ -97,6 +98,8 @@ public class SolrExampleJettyTest extends SolrExampleTests {
src = (String) doc.getFieldValue("_src_");
m = (Map) fromJSONString(src);
assertEquals("name2", m.get("name"));
+ } finally {
+ HttpClientUtil.close(httpClient);
}
}
diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/BasicHttpSolrClientTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/BasicHttpSolrClientTest.java
index 8bd9a193eed..0b299f34b4c 100644
--- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/BasicHttpSolrClientTest.java
+++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/BasicHttpSolrClientTest.java
@@ -649,10 +649,10 @@ public class BasicHttpSolrClientTest extends SolrJettyTestBase {
@Test
public void testGetRawStream() throws SolrServerException, IOException {
- CloseableHttpClient client = HttpClientUtil.createClient(null);
+ CloseableHttpClient httpClient = HttpClientUtil.createClient(null);
try (SolrClient solrClient =
new HttpSolrClient.Builder(getCoreUrl())
- .withHttpClient(client)
+ .withHttpClient(httpClient)
.withResponseParser(null)
.build(); ) {
@@ -661,6 +661,8 @@ public class BasicHttpSolrClientTest extends SolrJettyTestBase {
InputStream stream = (InputStream) response.get("stream");
assertNotNull(stream);
stream.close();
+ } finally {
+ HttpClientUtil.close(httpClient);
}
}
diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/Http2SolrClientTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/Http2SolrClientTest.java
index 3657906ec41..21800c6d5a9 100644
--- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/Http2SolrClientTest.java
+++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/Http2SolrClientTest.java
@@ -17,7 +17,10 @@
package org.apache.solr.client.solrj.impl;
+import static org.hamcrest.CoreMatchers.instanceOf;
+
import java.io.IOException;
+import java.io.InputStream;
import java.nio.charset.StandardCharsets;
import java.util.ArrayList;
import java.util.Base64;
@@ -56,6 +59,7 @@ import org.eclipse.jetty.client.WWWAuthenticationProtocolHandler;
import org.eclipse.jetty.http.HttpStatus;
import org.eclipse.jetty.servlet.ServletHolder;
import org.eclipse.jetty.util.ssl.SslContextFactory;
+import org.hamcrest.MatcherAssert;
import org.junit.BeforeClass;
import org.junit.Test;
@@ -947,7 +951,10 @@ public class Http2SolrClientTest extends SolrJettyTestBase {
new GenericSolrRequest(SolrRequest.METHOD.GET, "/select", params("q", "*:*"));
req.setResponseParser(new InputStreamResponseParser("xml"));
SimpleSolrResponse rsp = req.process(client);
- assertNotNull(rsp.getResponse().get("stream"));
+ Object stream = rsp.getResponse().get("stream");
+ assertNotNull(stream);
+ MatcherAssert.assertThat(stream, instanceOf(InputStream.class));
+ org.apache.solr.common.util.IOUtils.closeQuietly((InputStream) stream);
}
}
@@ -958,31 +965,34 @@ public class Http2SolrClientTest extends SolrJettyTestBase {
.withBasicAuthCredentials("testu", "testp")
.build()) {
- Http2SolrClient clone1 =
- new Http2SolrClient.Builder("baseSolrUrl").withHttpClient(seed).build();
- String expected1 =
- Http2SolrClient.basicAuthCredentialsToAuthorizationString("testu", "testp");
- assertEquals(expected1, clone1.basicAuthAuthorizationStr);
+ try (Http2SolrClient clone1 =
+ new Http2SolrClient.Builder("baseSolrUrl").withHttpClient(seed).build()) {
+ String expected1 =
+ Http2SolrClient.basicAuthCredentialsToAuthorizationString("testu", "testp");
+ assertEquals(expected1, clone1.basicAuthAuthorizationStr);
+ }
// test overwrite seed value
- Http2SolrClient clone2 =
+ try (Http2SolrClient clone2 =
new Http2SolrClient.Builder("baseSolrUrl")
.withHttpClient(seed)
.withBasicAuthCredentials("testu2", "testp2")
- .build();
- String expected2 =
- Http2SolrClient.basicAuthCredentialsToAuthorizationString("testu2", "testp2");
- assertEquals(expected2, clone2.basicAuthAuthorizationStr);
+ .build()) {
+ String expected2 =
+ Http2SolrClient.basicAuthCredentialsToAuthorizationString("testu2", "testp2");
+ assertEquals(expected2, clone2.basicAuthAuthorizationStr);
+ }
// test overwrite seed value, order of builder method calls reversed
- Http2SolrClient clone3 =
+ try (Http2SolrClient clone3 =
new Http2SolrClient.Builder("baseSolrUrl")
.withBasicAuthCredentials("testu3", "testp3")
.withHttpClient(seed)
- .build();
- String expected3 =
- Http2SolrClient.basicAuthCredentialsToAuthorizationString("testu3", "testp3");
- assertEquals(expected3, clone3.basicAuthAuthorizationStr);
+ .build()) {
+ String expected3 =
+ Http2SolrClient.basicAuthCredentialsToAuthorizationString("testu3", "testp3");
+ assertEquals(expected3, clone3.basicAuthAuthorizationStr);
+ }
}
}
diff --git a/solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java b/solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java
index 32a629824cc..111e2c4580d 100644
--- a/solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java
+++ b/solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java
@@ -109,7 +109,6 @@ import org.apache.solr.common.util.ContentStream;
import org.apache.solr.common.util.ContentStreamBase;
import org.apache.solr.common.util.ExecutorUtil;
import org.apache.solr.common.util.IOUtils;
-import org.apache.solr.common.util.ObjectReleaseTracker;
import org.apache.solr.common.util.SolrNamedThreadFactory;
import org.apache.solr.common.util.SuppressForbidden;
import org.apache.solr.common.util.Utils;
@@ -335,7 +334,6 @@ public abstract class SolrTestCaseJ4 extends SolrTestCase {
resetFactory();
coreName = DEFAULT_TEST_CORENAME;
} finally {
- ObjectReleaseTracker.clear();
TestInjection.reset();
initCoreDataDir = null;
System.clearProperty("solr.v2RealPath");
@@ -2633,7 +2631,13 @@ public abstract class SolrTestCaseJ4 extends SolrTestCase {
}
}
- /** This method creates a HttpClient from a URL. */
+ /**
+ * This method creates a HttpClient from a URL.
+ *
+ * <p><b>WARNING:</b> if you use this method, the <code>HttpClient</code> returned is tracked by
+ * <code>ObjectReleaseTracker</code>. Your test will fail if you do not pass the <code>HttpClient
+ * </code> to {@link HttpClientUtil#close(HttpClient)} when you are done with it.
+ */
@Deprecated // We are migrating away from Apache HttpClient.
public static HttpClient getHttpClient(String url) {
return new HttpSolrClient.Builder(url).build().getHttpClient();