Copilot commented on code in PR #4655:
URL: https://github.com/apache/solr/pull/4655#discussion_r3647695986
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpJdkSolrClientTest.java:
##########
@@ -637,6 +638,51 @@ public void testMaybeTryHeadRequestHasContentType() throws
Exception {
}
}
+ @Test(timeout = 30000)
+ public void testConcurrentStreamedBodiesDoNotDeadlockWithHttp1() throws
Exception {
+ DebugServlet.clear();
+ String url = solrTestRule.getBaseUrl() + DEBUG_SERVLET_PATH;
+
+ int concurrency = 8;
+ ExecutorService callers =
+ ExecutorUtil.newMDCAwareFixedThreadPool(concurrency, new
NamedThreadFactory("test-caller"));
+
+ try (HttpJdkSolrClient client = builder(url).useHttp1_1(true).build()) {
+ List<CompletableFuture<Void>> futures = new ArrayList<>(concurrency);
+ for (int i = 0; i < concurrency; i++) {
+ futures.add(
+ CompletableFuture.runAsync(
+ () -> {
+ JsonQueryRequest q = buildLargeBodyQuery();
+ try {
+ q.process(client);
+ } catch (Exception ignored) {
+ }
+ },
+ callers));
+ }
+ CompletableFuture.allOf(futures.toArray(new CompletableFuture<?>[0]))
+ .get(45, TimeUnit.SECONDS);
Review Comment:
The test method timeout is 30s, but the future wait timeout is 45s; in
practice JUnit will time out first, producing a less-informative failure and
potentially skipping cleanup. Keep the inner get() timeout comfortably below
the `@Test` timeout.
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpJdkSolrClientTest.java:
##########
@@ -637,6 +638,51 @@ public void testMaybeTryHeadRequestHasContentType() throws
Exception {
}
}
+ @Test(timeout = 30000)
+ public void testConcurrentStreamedBodiesDoNotDeadlockWithHttp1() throws
Exception {
+ DebugServlet.clear();
+ String url = solrTestRule.getBaseUrl() + DEBUG_SERVLET_PATH;
+
+ int concurrency = 8;
+ ExecutorService callers =
+ ExecutorUtil.newMDCAwareFixedThreadPool(concurrency, new
NamedThreadFactory("test-caller"));
+
+ try (HttpJdkSolrClient client = builder(url).useHttp1_1(true).build()) {
+ List<CompletableFuture<Void>> futures = new ArrayList<>(concurrency);
+ for (int i = 0; i < concurrency; i++) {
+ futures.add(
+ CompletableFuture.runAsync(
+ () -> {
+ JsonQueryRequest q = buildLargeBodyQuery();
+ try {
+ q.process(client);
+ } catch (Exception ignored) {
+ }
Review Comment:
The test swallows all exceptions from q.process(...), so it can pass even if
requests are failing immediately (masking regressions unrelated to deadlock).
Let the CompletableFuture complete exceptionally so allOf().get(...) fails when
a request fails.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java:
##########
@@ -117,26 +126,34 @@ protected HttpJdkSolrClient(String serverBaseUrl,
HttpJdkSolrClient.Builder buil
new SolrNamedThreadFactory(this.getClass().getSimpleName()));
this.shutdownExecutor = true;
}
- b.executor(this.executor);
+ this.httpClientExecutor =
+ new ExecutorUtil.MDCAwareThreadPoolExecutor(
+ 0,
+ Integer.MAX_VALUE,
+ 60,
+ TimeUnit.SECONDS,
+ new SynchronousQueue<>(),
+ new SolrNamedThreadFactory(this.getClass().getSimpleName() +
"-http"));
+ httpClientBuilder.executor(this.httpClientExecutor);
Review Comment:
httpClientExecutor is configured exactly like
ExecutorUtil.newMDCAwareCachedThreadPool(...). Using the helper avoids
duplicating the cached-thread-pool parameters and keeps this aligned with any
future changes to ExecutorUtil's implementation.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
