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 22:34:52 UTC
[solr] branch branch_9x 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 branch_9x
in repository https://gitbox.apache.org/repos/asf/solr.git
The following commit(s) were added to refs/heads/branch_9x by this push:
new 665070b97d8 SOLR-16983: Fixed a bug that could cause some usages of SolrStream to fail to close InputStreams from the server.
665070b97d8 is described below
commit 665070b97d8fee30f3dd8c57b7d02791b3bf3ba6
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
(cherry picked from commit 117a6cdf415ae43e3279aa1ea91adc8180a1cf74)
---
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 1b73db489df..395d6a9f292 100644
--- a/solr/CHANGES.txt
+++ b/solr/CHANGES.txt
@@ -138,6 +138,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 329cef1b13b..b3bf805af8e 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 1d5b6f95cf8..c41af634df7 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
}
@@ -1476,4 +1474,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 493a05cab0a..a9585e5fc04 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 c15bd3029fa..9b909b6fb2d 100644
--- a/solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java
+++ b/solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java
@@ -115,7 +115,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;
@@ -369,7 +368,6 @@ public abstract class SolrTestCaseJ4 extends SolrTestCase {
resetFactory();
coreName = DEFAULT_TEST_CORENAME;
} finally {
- ObjectReleaseTracker.clear();
TestInjection.reset();
initCoreDataDir = null;
System.clearProperty("solr.v2RealPath");
@@ -2670,7 +2668,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();