Re: [PR] SOLR-18174 AsyncTracker Semaphore leak reproduction [solr]
dsmiley commented on code in PR #4236:
URL: https://github.com/apache/solr/pull/4236#discussion_r3083855591
##
solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java:
##
@@ -440,5 +443,20 @@ public void initializeMetrics(SolrMetricsContext
parentContext, Attributes attri
commExecutor =
solrMetricsContext.instrumentedExecutorService(
commExecutor, "solr.core.executor", "httpShardExecutor",
SolrInfoBean.Category.QUERY);
+if (defaultClient != null) {
+ asyncRequestsGauge =
+ solrMetricsContext.observableLongGauge(
+ "solr.http.client.async_permits",
Review Comment:
See
https://github.com/apache/solr/blob/2c55000296bb04e12e2bfae83eca7e5f11caba0e/solr/core/src/java/org/apache/solr/util/stats/InstrumentedHttpListenerFactory.java
but I'm a bit unclear what those names look like in practice... I see it's
underscore based and no dots so I'm confused admittedly. I know there have
been some dot/underscore transformations and I forget where things stand.
--
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]
Re: [PR] SOLR-18174 AsyncTracker Semaphore leak reproduction [solr]
janhoy commented on code in PR #4236:
URL: https://github.com/apache/solr/pull/4236#discussion_r3080895693
##
solr/core/src/test/org/apache/solr/handler/component/AsyncTrackerSemaphoreLeakTest.java:
##
@@ -0,0 +1,481 @@
+/*
+ * 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.handler.component;
+
+import java.io.IOException;
+import java.lang.invoke.MethodHandles;
+import java.lang.reflect.Field;
+import java.net.ServerSocket;
+import java.net.Socket;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.Phaser;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import org.apache.lucene.util.SuppressForbidden;
+import org.apache.solr.client.solrj.impl.LBSolrClient;
+import org.apache.solr.client.solrj.jetty.HttpJettySolrClient;
+import org.apache.solr.client.solrj.jetty.LBJettySolrClient;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.client.solrj.request.QueryRequest;
+import org.apache.solr.cloud.SolrCloudTestCase;
+import org.apache.solr.common.params.ModifiableSolrParams;
+import org.eclipse.jetty.client.Request;
+import org.eclipse.jetty.client.Response;
+import org.eclipse.jetty.client.Result;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.mockito.ArgumentMatchers;
+import org.mockito.Mockito;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Tests for two semaphore-permit leak bugs in {@link HttpJettySolrClient}'s
{@code AsyncTracker}
+ * that cause distributed queries to hang permanently.
+ *
+ * Pattern A – HTTP/2 GOAWAY double-queue leak
+ *
+ * Jetty HTTP/2 can re-queue the same exchange after a GOAWAY/connection
race, firing {@code
+ * onRequestQueued} twice for one logical request. Because {@code onComplete}
fires only once, one
+ * permit is permanently consumed per occurrence, gradually draining the
semaphore over hours or
+ * days until Pattern B triggers.
+ *
+ * Pattern B – IO-thread deadlock on LB retry when permits depleted
+ *
+ * When a connection-level failure causes {@link
+ * org.apache.solr.client.solrj.jetty.LBJettySolrClient} to retry
synchronously inside a {@code
+ * whenComplete} callback on the Jetty IO selector thread, the retry calls
{@code acquire()} on that
+ * same IO thread before the original request's {@code onComplete} can call
{@code release()}. No
+ * permits are permanently lost — the deadlock simply requires two permits to
be available
+ * simultaneously — but if the semaphore is at zero, {@code acquire()} blocks
the IO thread
+ * permanently and distributed queries hang forever.
+ */
+public class AsyncTrackerSemaphoreLeakTest extends SolrCloudTestCase {
+
+ private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ private static final String COLLECTION = "semaphore_leak_test";
+
+ /** Reduced semaphore size so we can observe the drain without needing
thousands of requests. */
+ private static final int MAX_PERMITS = 40;
+
+ /**
+ * Number of concurrent requests. Set equal to MAX_PERMITS so that all
permits are exhausted
+ * before any retry can acquire, triggering the IO-thread deadlock.
+ */
+ private static final int NUM_RETRY_REQUESTS = MAX_PERMITS;
+
+ @BeforeClass
+ public static void setupCluster() throws Exception {
+// Reduce the semaphore size so we can observe drain with few requests.
+// This property is read when HttpJettySolrClient is constructed, so it
must
+// be set BEFORE the cluster (and its HttpShardHandlerFactory) starts up.
+System.setProperty(HttpJettySolrClient.ASYNC_REQUESTS_MAX_SYSPROP,
String.valueOf(MAX_PERMITS));
+
+configureCluster(2).addConfig("conf",
configset("cloud-dynamic")).configure();
+
+CollectionAdminRequest.createCollection(COLLECTION, "conf", 2, 1)
+.process(cluster.getSolrClient());
+
+waitForState(
+"Expected 1 active s
Re: [PR] SOLR-18174 AsyncTracker Semaphore leak reproduction [solr]
janhoy commented on code in PR #4236:
URL: https://github.com/apache/solr/pull/4236#discussion_r3080562414
##
solr/core/src/test/org/apache/solr/handler/component/AsyncTrackerSemaphoreLeakTest.java:
##
@@ -0,0 +1,481 @@
+/*
+ * 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.handler.component;
+
+import java.io.IOException;
+import java.lang.invoke.MethodHandles;
+import java.lang.reflect.Field;
+import java.net.ServerSocket;
+import java.net.Socket;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.Phaser;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import org.apache.lucene.util.SuppressForbidden;
+import org.apache.solr.client.solrj.impl.LBSolrClient;
+import org.apache.solr.client.solrj.jetty.HttpJettySolrClient;
+import org.apache.solr.client.solrj.jetty.LBJettySolrClient;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.client.solrj.request.QueryRequest;
+import org.apache.solr.cloud.SolrCloudTestCase;
+import org.apache.solr.common.params.ModifiableSolrParams;
+import org.eclipse.jetty.client.Request;
+import org.eclipse.jetty.client.Response;
+import org.eclipse.jetty.client.Result;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.mockito.ArgumentMatchers;
+import org.mockito.Mockito;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Tests for two semaphore-permit leak bugs in {@link HttpJettySolrClient}'s
{@code AsyncTracker}
+ * that cause distributed queries to hang permanently.
+ *
+ * Pattern A – HTTP/2 GOAWAY double-queue leak
+ *
+ * Jetty HTTP/2 can re-queue the same exchange after a GOAWAY/connection
race, firing {@code
+ * onRequestQueued} twice for one logical request. Because {@code onComplete}
fires only once, one
+ * permit is permanently consumed per occurrence, gradually draining the
semaphore over hours or
+ * days until Pattern B triggers.
+ *
+ * Pattern B – IO-thread deadlock on LB retry when permits depleted
+ *
+ * When a connection-level failure causes {@link
+ * org.apache.solr.client.solrj.jetty.LBJettySolrClient} to retry
synchronously inside a {@code
+ * whenComplete} callback on the Jetty IO selector thread, the retry calls
{@code acquire()} on that
+ * same IO thread before the original request's {@code onComplete} can call
{@code release()}. No
+ * permits are permanently lost — the deadlock simply requires two permits to
be available
+ * simultaneously — but if the semaphore is at zero, {@code acquire()} blocks
the IO thread
+ * permanently and distributed queries hang forever.
+ */
+public class AsyncTrackerSemaphoreLeakTest extends SolrCloudTestCase {
+
+ private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ private static final String COLLECTION = "semaphore_leak_test";
+
+ /** Reduced semaphore size so we can observe the drain without needing
thousands of requests. */
+ private static final int MAX_PERMITS = 40;
+
+ /**
+ * Number of concurrent requests. Set equal to MAX_PERMITS so that all
permits are exhausted
+ * before any retry can acquire, triggering the IO-thread deadlock.
+ */
+ private static final int NUM_RETRY_REQUESTS = MAX_PERMITS;
+
+ @BeforeClass
+ public static void setupCluster() throws Exception {
+// Reduce the semaphore size so we can observe drain with few requests.
+// This property is read when HttpJettySolrClient is constructed, so it
must
+// be set BEFORE the cluster (and its HttpShardHandlerFactory) starts up.
+System.setProperty(HttpJettySolrClient.ASYNC_REQUESTS_MAX_SYSPROP,
String.valueOf(MAX_PERMITS));
+
+configureCluster(2).addConfig("conf",
configset("cloud-dynamic")).configure();
Review Comment:
I suspect it might be sufficient with one node, as we can fit 2 shards on
one node, and all our code paths will be triggered, even on the same nod
Re: [PR] SOLR-18174 AsyncTracker Semaphore leak reproduction [solr]
janhoy commented on code in PR #4236:
URL: https://github.com/apache/solr/pull/4236#discussion_r3080523580
##
solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java:
##
@@ -440,5 +443,20 @@ public void initializeMetrics(SolrMetricsContext
parentContext, Attributes attri
commExecutor =
solrMetricsContext.instrumentedExecutorService(
commExecutor, "solr.core.executor", "httpShardExecutor",
SolrInfoBean.Category.QUERY);
+if (defaultClient != null) {
+ asyncRequestsGauge =
+ solrMetricsContext.observableLongGauge(
+ "solr.http.client.async_permits",
Review Comment:
No, I did not find a suitable namespace, suggestions?
--
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]
Re: [PR] SOLR-18174 AsyncTracker Semaphore leak reproduction [solr]
dsmiley commented on code in PR #4236:
URL: https://github.com/apache/solr/pull/4236#discussion_r3079560215
##
changelog/unreleased/SOLR-18174-prevent-double-registration.yml:
##
@@ -0,0 +1,8 @@
+title: "Fix semaphore permit leaks in HttpJettySolrClient's AsyncTracker.
Avoid IO-thread deadlock on connection failure retries. Add a new metric gauge
solr.http.client.async_permits"
Review Comment:
The double quotes are abnormal
##
solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java:
##
@@ -440,5 +443,20 @@ public void initializeMetrics(SolrMetricsContext
parentContext, Attributes attri
commExecutor =
solrMetricsContext.instrumentedExecutorService(
commExecutor, "solr.core.executor", "httpShardExecutor",
SolrInfoBean.Category.QUERY);
+if (defaultClient != null) {
+ asyncRequestsGauge =
+ solrMetricsContext.observableLongGauge(
+ "solr.http.client.async_permits",
Review Comment:
I don't see that we use this metrics namespace anywhere.
##
solr/solrj-jetty/src/java/org/apache/solr/client/solrj/jetty/HttpJettySolrClient.java:
##
@@ -110,6 +111,9 @@ public class HttpJettySolrClient extends HttpSolrClientBase
{
*/
public static final String CLIENT_CUSTOMIZER_SYSPROP =
"solr.solrj.http.jetty.customizer";
+ /** System property to cap the maximum number of outstanding async HTTP
requests. Default 1000. */
+ public static final String ASYNC_REQUESTS_MAX_SYSPROP =
"solr.http.client.async_requests.max";
Review Comment:
Why this sysprop namespace? The field above clearly shows a different
namespace.
##
solr/solrj-jetty/src/java/org/apache/solr/client/solrj/jetty/HttpJettySolrClient.java:
##
@@ -127,6 +131,16 @@ public class HttpJettySolrClient extends
HttpSolrClientBase {
private ExecutorService executor;
private boolean shutdownExecutor;
+ /** Fallback for {@code onFailure} dispatch; unbounded so it never rejects.
*/
+ private final ExecutorService failureDispatchExecutor =
+ new ExecutorUtil.MDCAwareThreadPoolExecutor(
+ 1,
Review Comment:
1? We're going to keep a thread around just in case? No; set this to 0
please. If an error happens, a thread might need to be created and that's fine.
##
solr/core/src/test/org/apache/solr/handler/component/AsyncTrackerSemaphoreLeakTest.java:
##
@@ -0,0 +1,481 @@
+/*
+ * 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.handler.component;
+
+import java.io.IOException;
+import java.lang.invoke.MethodHandles;
+import java.lang.reflect.Field;
+import java.net.ServerSocket;
+import java.net.Socket;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.Phaser;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import org.apache.lucene.util.SuppressForbidden;
+import org.apache.solr.client.solrj.impl.LBSolrClient;
+import org.apache.solr.client.solrj.jetty.HttpJettySolrClient;
+import org.apache.solr.client.solrj.jetty.LBJettySolrClient;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.client.solrj.request.QueryRequest;
+import org.apache.solr.cloud.SolrCloudTestCase;
+import org.apache.solr.common.params.ModifiableSolrParams;
+import org.eclipse.jetty.client.Request;
+import org.eclipse.jetty.client.Response;
+import org.eclipse.jetty.client.Result;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.mockito.ArgumentMatchers;
+import org.mockito.Mockito;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Tests for two semaphore-permit leak bugs in {@link HttpJettySolrClient}'s
{@code AsyncTracker}
+ * that cause distributed queries to hang permanently.
+ *
+ * Pattern A – HTTP/2 GOAWAY double-queue leak
+ *
+ * Jetty HTTP/2 can re-queue the same exchange after a GOAWAY/connection
race,
Re: [PR] SOLR-18174 AsyncTracker Semaphore leak reproduction [solr]
janhoy commented on code in PR #4236:
URL: https://github.com/apache/solr/pull/4236#discussion_r3078932675
##
solr/core/src/test/org/apache/solr/handler/component/AsyncTrackerSemaphoreLeakTest.java:
##
@@ -0,0 +1,564 @@
+/*
+ * 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.handler.component;
+
+import java.io.IOException;
+import java.lang.invoke.MethodHandles;
+import java.lang.reflect.Field;
+import java.net.ServerSocket;
+import java.net.Socket;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.Phaser;
+import java.util.concurrent.TimeUnit;
+import org.apache.lucene.util.SuppressForbidden;
+import org.apache.solr.client.solrj.impl.LBSolrClient;
+import org.apache.solr.client.solrj.jetty.HttpJettySolrClient;
+import org.apache.solr.client.solrj.jetty.LBJettySolrClient;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.client.solrj.request.QueryRequest;
+import org.apache.solr.cloud.SolrCloudTestCase;
+import org.apache.solr.common.params.ModifiableSolrParams;
+import org.eclipse.jetty.client.Request;
+import org.eclipse.jetty.client.Response;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.mockito.ArgumentMatchers;
+import org.mockito.Mockito;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Reproduces the {@link org.apache.solr.client.solrj.impl.LBAsyncSolrClient}
semaphore leak that
+ * causes distributed queries to hang permanently.
+ *
+ * Bug scenario
+ *
+ *
+ * A shard HTTP request fails with a connection-level error
(not an HTTP-level
+ * error). Jetty fires the {@code onFailure} response callback directly
on the IO selector
+ * thread.
+ * {@link
org.apache.solr.client.solrj.jetty.HttpJettySolrClient#requestAsync} completes
its
+ * {@code CompletableFuture} exceptionally from within that {@code
onFailure} callback — still
+ * on the IO thread.
+ * {@code LBAsyncSolrClient.doAsyncRequest} registered a {@code
whenComplete} on that future.
+ * Because the future completes on the IO thread, {@code whenComplete}
also fires
+ * synchronously on the IO thread.
+ * The {@code whenComplete} action calls {@code doAsyncRequest} again
(retry to the next
+ * endpoint), which eventually calls Jetty's {@code HttpClient.send()}.
That triggers the
+ * {@code AsyncTracker.queuedListener} — which calls {@code
semaphore.acquire()} — still on
+ * the IO thread, before the original request's {@code
completeListener.onComplete()} has had
+ * a chance to call {@code semaphore.release()}.
+ * If the semaphore is at zero, {@code acquire()} blocks the IO
thread. The blocked
+ * IO thread cannot execute the {@code completeListener} that would
release the original
+ * permit. The permit is permanently leaked, and the IO thread is
permanently stuck. Repeat
+ * until all permits are exhausted: distributed queries hang forever.
+ *
+ *
+ * Test setup
+ *
+ * A raw TCP server accepts {@value #NUM_RETRY_REQUESTS} connections and
holds them open until
+ * all are established (so all semaphore permits are consumed). It then closes
all connections
+ * simultaneously via TCP RST, causing all Jetty {@code onFailure} events to
fire on the IO threads
+ * at the same time. Because the semaphore is already at 0, every retry's
{@code acquire()} blocks
+ * the IO thread immediately, and no {@code onComplete} release can fire.
+ *
+ * The test asserts that after a short wait the semaphore is at 0 and none
of the {@code
+ * CompletableFuture}s returned by {@code requestAsync} have completed —
proving the permanent
+ * permit exhaustion.
+ */
+public class AsyncTrackerSemaphoreLeakTest extends SolrCloudTestCase {
+
+ private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ private static final String COLLECTION = "semaphore_leak_test";
+
+ /** Reduced semaphore size so we can obse
Re: [PR] SOLR-18174 AsyncTracker Semaphore leak reproduction [solr]
janhoy commented on code in PR #4236:
URL: https://github.com/apache/solr/pull/4236#discussion_r3078918385
##
solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java:
##
@@ -440,5 +443,20 @@ public void initializeMetrics(SolrMetricsContext
parentContext, Attributes attri
commExecutor =
solrMetricsContext.instrumentedExecutorService(
commExecutor, "solr.core.executor", "httpShardExecutor",
SolrInfoBean.Category.QUERY);
+if (defaultClient != null) {
+ asyncRequestsGauge =
+ solrMetricsContext.observableLongGauge(
+ "solr.http.client.async_permits",
+ "Outstanding async HTTP request permits in the Jetty SolrJ
client"
+ + " (state=max: configured ceiling; state=available:
currently unused permits).",
Review Comment:
Fixed PR description
--
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]
Re: [PR] SOLR-18174 AsyncTracker Semaphore leak reproduction [solr]
janhoy commented on code in PR #4236:
URL: https://github.com/apache/solr/pull/4236#discussion_r3078776157
##
solr/solrj-jetty/src/java/org/apache/solr/client/solrj/jetty/HttpJettySolrClient.java:
##
@@ -844,28 +870,48 @@ private static class AsyncTracker {
private final Response.CompleteListener completeListener;
AsyncTracker() {
+ maxRequests = Integer.getInteger(ASYNC_REQUESTS_MAX_SYSPROP,
MAX_OUTSTANDING_REQUESTS);
Review Comment:
Nah, let people shoot themselves in the foot...
--
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]
Re: [PR] SOLR-18174 AsyncTracker Semaphore leak reproduction [solr]
janhoy commented on code in PR #4236:
URL: https://github.com/apache/solr/pull/4236#discussion_r3078695661
##
solr/core/src/test/org/apache/solr/handler/component/AsyncTrackerSemaphoreLeakTest.java:
##
@@ -0,0 +1,564 @@
+/*
+ * 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.handler.component;
+
+import java.io.IOException;
+import java.lang.invoke.MethodHandles;
+import java.lang.reflect.Field;
+import java.net.ServerSocket;
+import java.net.Socket;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.Phaser;
+import java.util.concurrent.TimeUnit;
+import org.apache.lucene.util.SuppressForbidden;
+import org.apache.solr.client.solrj.impl.LBSolrClient;
+import org.apache.solr.client.solrj.jetty.HttpJettySolrClient;
+import org.apache.solr.client.solrj.jetty.LBJettySolrClient;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.client.solrj.request.QueryRequest;
+import org.apache.solr.cloud.SolrCloudTestCase;
+import org.apache.solr.common.params.ModifiableSolrParams;
+import org.eclipse.jetty.client.Request;
+import org.eclipse.jetty.client.Response;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.mockito.ArgumentMatchers;
+import org.mockito.Mockito;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Reproduces the {@link org.apache.solr.client.solrj.impl.LBAsyncSolrClient}
semaphore leak that
+ * causes distributed queries to hang permanently.
+ *
+ * Bug scenario
+ *
+ *
+ * A shard HTTP request fails with a connection-level error
(not an HTTP-level
+ * error). Jetty fires the {@code onFailure} response callback directly
on the IO selector
+ * thread.
+ * {@link
org.apache.solr.client.solrj.jetty.HttpJettySolrClient#requestAsync} completes
its
+ * {@code CompletableFuture} exceptionally from within that {@code
onFailure} callback — still
+ * on the IO thread.
+ * {@code LBAsyncSolrClient.doAsyncRequest} registered a {@code
whenComplete} on that future.
+ * Because the future completes on the IO thread, {@code whenComplete}
also fires
+ * synchronously on the IO thread.
+ * The {@code whenComplete} action calls {@code doAsyncRequest} again
(retry to the next
+ * endpoint), which eventually calls Jetty's {@code HttpClient.send()}.
That triggers the
+ * {@code AsyncTracker.queuedListener} — which calls {@code
semaphore.acquire()} — still on
+ * the IO thread, before the original request's {@code
completeListener.onComplete()} has had
+ * a chance to call {@code semaphore.release()}.
+ * If the semaphore is at zero, {@code acquire()} blocks the IO
thread. The blocked
+ * IO thread cannot execute the {@code completeListener} that would
release the original
+ * permit. The permit is permanently leaked, and the IO thread is
permanently stuck. Repeat
+ * until all permits are exhausted: distributed queries hang forever.
+ *
+ *
+ * Test setup
+ *
+ * A raw TCP server accepts {@value #NUM_RETRY_REQUESTS} connections and
holds them open until
+ * all are established (so all semaphore permits are consumed). It then closes
all connections
+ * simultaneously via TCP RST, causing all Jetty {@code onFailure} events to
fire on the IO threads
+ * at the same time. Because the semaphore is already at 0, every retry's
{@code acquire()} blocks
+ * the IO thread immediately, and no {@code onComplete} release can fire.
+ *
+ * The test asserts that after a short wait the semaphore is at 0 and none
of the {@code
+ * CompletableFuture}s returned by {@code requestAsync} have completed —
proving the permanent
+ * permit exhaustion.
+ */
+public class AsyncTrackerSemaphoreLeakTest extends SolrCloudTestCase {
+
+ private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ private static final String COLLECTION = "semaphore_leak_test";
+
+ /** Reduced semaphore size so we can obse
Re: [PR] SOLR-18174 AsyncTracker Semaphore leak reproduction [solr]
janhoy commented on code in PR #4236:
URL: https://github.com/apache/solr/pull/4236#discussion_r3078522823
##
solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java:
##
@@ -440,5 +443,20 @@ public void initializeMetrics(SolrMetricsContext
parentContext, Attributes attri
commExecutor =
solrMetricsContext.instrumentedExecutorService(
commExecutor, "solr.core.executor", "httpShardExecutor",
SolrInfoBean.Category.QUERY);
+if (defaultClient != null) {
+ asyncRequestsGauge =
+ solrMetricsContext.observableLongGauge(
+ "solr.http.client.async_permits",
+ "Outstanding async HTTP request permits in the Jetty SolrJ
client"
+ + " (state=max: configured ceiling; state=available:
currently unused permits).",
Review Comment:
It was renamed to `solr.http.client.async_permits` as it felt more
descriptive. But perhaps it is self explanatory that the original
`solr.http.client.async_requests` with labels `max` and `available` is about
request permits?
--
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]
Re: [PR] SOLR-18174 AsyncTracker Semaphore leak reproduction [solr]
janhoy commented on PR #4236: URL: https://github.com/apache/solr/pull/4236#issuecomment-4242851188 New development: I installed an instrumented version of Solr in client test environment, where the deadlock had occurred last time after about two weeks. The instrumented version would print additional log lines for semaphore statistics, thread stats and try to detect leaks by monitoring which threads did not release their permit. It would also print error logs for the two suspected code paths which are patched in this PR: - Jetty's double registration of `onRequestQueued` - CompleatableFuture retry path Here is a sample of some log prints ``` 14.4.2026 08:59:38 WARN Http2SolrClient$AsyncTracker event=async_tracker_stats permits=1000 permits_max=1000 permits_used=0 inflight=0 net_outstanding=0 acquires_total=2811 releases_total=2811 threads_running=19 threads_waiting=88 threads_timed_waiting=20 threads_blocked=0 threads_total=127 14.4.2026 09:00:38 WARN Http2SolrClient$AsyncTracker event=async_tracker_stats permits=1000 permits_max=1000 permits_used=0 inflight=0 net_outstanding=0 acquires_total=2811 releases_total=2811 threads_running=19 threads_waiting=88 threads_timed_waiting=20 threads_blocked=0 threads_total=127 14.4.2026 09:01:12 ERROR Http2SolrClient$AsyncTracker event=double_registration_prevented method=POST url="http://my-host:8983/solr/my-collection_shard1_replica_n6/select"; permits_available=998 permits_max=1000 msg="Jetty fired queuedListener twice for same Request — permit leak prevented by idempotency guard" 14.4.2026 09:01:12 ERROR Http2SolrClient$AsyncTracker event=double_registration_prevented method=POST url="http://my-host:8983/solr/my-collection_shard1_replica_n6/select"; permits_available=998 permits_max=1000 msg="Jetty fired queuedListener twice for same Request — permit leak prevented by idempotency guard" 14.4.2026 09:01:38 WARN Http2SolrClient$AsyncTracker event=async_tracker_stats permits=1000 permits_max=1000 permits_used=0 inflight=0 net_outstanding=0 acquires_total=2815 releases_total=2815 threads_running=19 threads_waiting=87 threads_timed_waiting=21 threads_blocked=0 threads_total=127 14.4.2026 09:02:38 WARN Http2SolrClient$AsyncTracker event=async_tracker_stats permits=1000 permits_max=1000 permits_used=0 inflight=0 net_outstanding=0 acquires_total=2815 releases_total=2815 threads_running=19 threads_waiting=87 threads_timed_waiting=21 threads_blocked=0 threads_total=127 ``` This cluster has been running with some test traffic, in a real k8s env with linkerd mesh, for some 3 days. And during that time the `double_registration_prevented` event occurred 223 times. This rhymes well with full depletion of the 1000 permits during two weeks that we experienced last time (223/3*14=9217). https://github.com/user-attachments/assets/b0fa6509-df58-457d-a4c2-c0112d1a0e0f"; /> So that is a strong indication that the Jetty doble firing was the main root cause in our case. I'll clean up this PR branch and make it ready for merge and back-port. This PR included - Fix the the Jetty double-fire issue by adding PERMIT_ACQUIRED_ATTR Idempotency guard - Dispatch error-retry in CompletableFuture to an executor - Add a new metric gauge to keep an eye on async permits: `solr.http.client.async_permits` - Make the semaphore size configurable through sysprop `solr.http.client.async_requests.max` I plan to merge during this week unless concerns are voiced -- 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]
Re: [PR] SOLR-18174 AsyncTracker Semaphore leak reproduction [solr]
Copilot commented on code in PR #4236:
URL: https://github.com/apache/solr/pull/4236#discussion_r3078428236
##
solr/solrj-jetty/src/java/org/apache/solr/client/solrj/jetty/HttpJettySolrClient.java:
##
@@ -439,7 +443,17 @@ public void onHeaders(Response response) {
@Override
public void onFailure(Response response, Throwable failure) {
super.onFailure(response, failure);
-future.completeExceptionally(new
SolrServerException(failure.getMessage(), failure));
+// Dispatch off the IO thread so any whenComplete retry won't
block on
+// semaphore.acquire().
+try {
+ executor.execute(
+ () ->
+ future.completeExceptionally(
+ new SolrServerException(failure.getMessage(),
failure)));
+} catch (RejectedExecutionException ree) {
+ // Executor shut down; safe to complete inline since retries
will fail immediately.
+ future.completeExceptionally(new
SolrServerException(failure.getMessage(), failure));
+}
Review Comment:
The RejectedExecutionException fallback completes the future inline on the
Jetty IO thread, which can reintroduce the same deadlock when the executor is
saturated (not only when it’s shut down). RejectedExecutionException is also
thrown on queue/full saturation for the default MDCAwareThreadPoolExecutor.
Consider always completing off the IO thread (e.g., dispatch to a different
always-available executor as fallback) rather than completing inline here.
##
solr/core/src/test/org/apache/solr/handler/component/AsyncTrackerSemaphoreLeakTest.java:
##
@@ -0,0 +1,564 @@
+/*
+ * 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.handler.component;
+
+import java.io.IOException;
+import java.lang.invoke.MethodHandles;
+import java.lang.reflect.Field;
+import java.net.ServerSocket;
+import java.net.Socket;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.Phaser;
+import java.util.concurrent.TimeUnit;
+import org.apache.lucene.util.SuppressForbidden;
+import org.apache.solr.client.solrj.impl.LBSolrClient;
+import org.apache.solr.client.solrj.jetty.HttpJettySolrClient;
+import org.apache.solr.client.solrj.jetty.LBJettySolrClient;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.client.solrj.request.QueryRequest;
+import org.apache.solr.cloud.SolrCloudTestCase;
+import org.apache.solr.common.params.ModifiableSolrParams;
+import org.eclipse.jetty.client.Request;
+import org.eclipse.jetty.client.Response;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.mockito.ArgumentMatchers;
+import org.mockito.Mockito;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Reproduces the {@link org.apache.solr.client.solrj.impl.LBAsyncSolrClient}
semaphore leak that
+ * causes distributed queries to hang permanently.
+ *
+ * Bug scenario
+ *
+ *
+ * A shard HTTP request fails with a connection-level error
(not an HTTP-level
+ * error). Jetty fires the {@code onFailure} response callback directly
on the IO selector
+ * thread.
+ * {@link
org.apache.solr.client.solrj.jetty.HttpJettySolrClient#requestAsync} completes
its
+ * {@code CompletableFuture} exceptionally from within that {@code
onFailure} callback — still
+ * on the IO thread.
+ * {@code LBAsyncSolrClient.doAsyncRequest} registered a {@code
whenComplete} on that future.
+ * Because the future completes on the IO thread, {@code whenComplete}
also fires
+ * synchronously on the IO thread.
+ * The {@code whenComplete} action calls {@code doAsyncRequest} again
(retry to the next
+ * endpoint), which eventually calls Jetty's {@code HttpClient.send()}.
That triggers the
+ * {@code AsyncTracker.queuedListener} — which calls {@code
semaphore.acquire()} — still on
+ * the IO
Re: [PR] SOLR-18174 AsyncTracker Semaphore leak reproduction [solr]
janhoy commented on PR #4236: URL: https://github.com/apache/solr/pull/4236#issuecomment-4130477206 > I guess this is only tangentially related, but there has to be a simpler way of doing all of this, right? This class is just hideously complex and tracing a single request is almost impossible with the number of CompletableFutures, and async method rabbit holes. Could this possibly be re-architected to be much simpler? Can we use utilities that Jetty gives us? I'm not saying the answer is yes, but I really think we ought to look into it, because this is a constant cause of bugs and headaches, and we are really just doing async requests using a library... Yea, agree that taking a step back and re-think is the right long term answer here. In 10.x we're on Java21 so we can use virtual threads now. I.e. replace CompletableFuture+Semaphore with plain blocking request() calls dispatched on Java virtual threads — one per shard. Simpler and cheap... But this is getting off topic. Someone should start a dev-list discussion and/or a SIP about such a redesign. But for fixing this in 9.x line, which will stil have a long life, we need to struggle with what we have, and at the least make semaphore max configurable, but probably other fixes as well. -- 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]
Re: [PR] SOLR-18174 AsyncTracker Semaphore leak reproduction [solr]
dsmiley commented on PR #4236: URL: https://github.com/apache/solr/pull/4236#issuecomment-4129625684 > I guess this is only tangentially related, but there has to be a simpler way of doing all of this, right? ... I thought I communicated this elsewhere but I agree and I think we shouldn't add a semaphore. I believe Dat basically ported what Apache HttpClient was doing over to Jetty HttpClient, including very questionably recreating some limits that Apache HttpClient natively had over to Jetty HttpClient which didn't have an identical limit. (Disclaimer: I could be mis-understanding!) IMO we should lean on whatever the HttpClient of choice is naturally offering us. If it has some sort of limit, cool. If not, that's cool too -- deal with it. Don't introduce new limits, or at the very least, default them off. CC @magibney -- 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]
Re: [PR] SOLR-18174 AsyncTracker Semaphore leak reproduction [solr]
HoustonPutman commented on PR #4236: URL: https://github.com/apache/solr/pull/4236#issuecomment-4129534274 > @HoustonPutman I ping you since you were bit by the request cancellation/abort bug in 9.x and I wonder if you could shed light on why `request.abort()` was also removed from 10.s line. @iamsanjay was seeing something similar after an upgrade of Jetty 12, so I assumed this bug made it over to that version of Jetty as well, and suggested that commenting it out had helped solve the issue in Solr 9. I guess this is only tangentially related, but there has to be a simpler way of doing all of this, right? This class is just hideously complex and tracing a single request is almost impossible with the number of CompletableFutures, and async method rabbit holes. Could this possibly be re-architected to be much simpler? Can we use utilities that Jetty gives us? I'm not saying the answer is yes, but I really think we ought to look into it, because this is a constant cause of bugs and headaches, and we are really just doing async requests using a library... -- 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]
Re: [PR] SOLR-18174 AsyncTracker Semaphore leak reproduction [solr]
janhoy commented on code in PR #4236:
URL: https://github.com/apache/solr/pull/4236#discussion_r2987175207
##
solr/solrj-jetty/src/java/org/apache/solr/client/solrj/jetty/HttpJettySolrClient.java:
##
@@ -844,28 +870,48 @@ private static class AsyncTracker {
private final Response.CompleteListener completeListener;
AsyncTracker() {
+ maxRequests = Integer.getInteger(ASYNC_REQUESTS_MAX_SYSPROP,
MAX_OUTSTANDING_REQUESTS);
// TODO: what about shared instances?
phaser = new Phaser(1);
- available = new Semaphore(MAX_OUTSTANDING_REQUESTS, false);
+ available = new Semaphore(maxRequests, false);
queuedListener =
request -> {
+if (request.getAttributes().get(PERMIT_ACQUIRED_ATTR) != null) {
+ return;
+}
phaser.register();
try {
available.acquire();
-} catch (InterruptedException ignored) {
-
+} catch (InterruptedException e) {
+ // Undo phaser registration: no permit was acquired so
completeListener must not
+ // release.
+ phaser.arriveAndDeregister();
+ Thread.currentThread().interrupt();
+ return;
}
+request.attribute(PERMIT_ACQUIRED_ATTR, Boolean.TRUE);
};
completeListener =
result -> {
-phaser.arriveAndDeregister();
-available.release();
+if (result == null
Review Comment:
Sometimes result is `null` here, so we can't fetch request attribute.
I wonder if this change here is not needed. The attribute's mission is to
avoid double acquire of permit, but is there also a potential double
`onComplete()` call to guard against?
--
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]
Re: [PR] SOLR-18174 AsyncTracker Semaphore leak reproduction [solr]
janhoy closed pull request #4236: SOLR-18174 AsyncTracker Semaphore leak reproduction URL: https://github.com/apache/solr/pull/4236 -- 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]
Re: [PR] SOLR-18174 AsyncTracker Semaphore leak reproduction [solr]
janhoy commented on code in PR #4236:
URL: https://github.com/apache/solr/pull/4236#discussion_r2986590763
##
solr/solrj-jetty/src/java/org/apache/solr/client/solrj/jetty/HttpJettySolrClient.java:
##
@@ -110,6 +110,12 @@ public class HttpJettySolrClient extends
HttpSolrClientBase {
*/
public static final String CLIENT_CUSTOMIZER_SYSPROP =
"solr.solrj.http.jetty.customizer";
+ /**
+ * System property to cap the maximum number of outstanding async HTTP
requests. Defaults to
+ * {@code 1000}. Lower values reduce memory pressure in resource-constrained
environments.
+ */
+ public static final String ASYNC_REQUESTS_MAX_SYSPROP =
"solr.http.client.async_requests.max";
Review Comment:
Making the semaphore max limit configurable was also discussed in #2313 but
rejected.. @magibney comments?
--
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]
Re: [PR] SOLR-18174 AsyncTracker Semaphore leak reproduction [solr]
janhoy commented on PR #4236: URL: https://github.com/apache/solr/pull/4236#issuecomment-4122024576 @dsmiley, @mlbiscoc, @kotman12, @HoustonPutman I'm not calling you out here to get a thorough review of this PR code or every theory in the mostly LLM generated analysis. But you are bright minds, and I fear that this issue is a series of bugs lurking, that will crop up once more heavy usage of newer solr releases reaches prime time. Perhaps some of you will connect some dots when seeing some of the code paths being discussed here. @HoustonPutman I ping you since you were bit by the request cancellation/abort bug in 9.x and I wonder if you could shed light on why `request.abort()` was also removed from 10.s line. The actual bug fixed in this PR is quite serious I believe. The LBSolrClient's retry request is executed synchronously on the IO selector thread instead of in the backgorund, thus enabling the deadlock when semaphore permits are depleted. My $1 question is how those permits leak in the first place, so I added a metric gauge for it. I suspect there is some code path acquiring a permit that is never released, but so far I have some LLM theories but no evidence. I may build a custom Solr 9.10.2-SNAPSHOT with added logging and instrumentation around the AsyncTracker and deploy it to our test cluster hoping for a reproduction, although it took 14 days of run time before it manifested last time... Thankful for advice on strategies for catching the root cause. Tricky thing is it may be related to complex servicemesh proxying and mass-interruption of open connections.. -- 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]
[PR] SOLR-18174 AsyncTracker Semaphore leak reproduction [solr]
janhoy opened a new pull request, #4236: URL: https://github.com/apache/solr/pull/4236 Relates to https://issues.apache.org/jira/browse/SOLR-18174 ## Background `HttpJettySolrClient` uses an internal `AsyncTracker` semaphore (default 1000 permits) to limit outstanding async HTTP requests. A permit is acquired in `onRequestQueued` and released in `onComplete`. Two separate bugs can cause permits to be consumed without being returned, eventually exhausting the semaphore. Once it reaches zero, every new distributed query blocks permanently on `semaphore.acquire()` — there is no timeout and no recovery short of a node restart. ## Two-phase failure model This PR aims to prove both phases through dedicated reproduction tests. So far we have only succeeded in proving the second phase, by simulating low available permits. What is shown as phase 1 in the diagram below is what we assume must be happening in real life, based on externally observed cluster behavior and clues in thread-dumps. https://github.com/user-attachments/assets/9ad07ead-16ac-4038-8987-9d9300e826bc"; /> ### Phase 1 — Gradual permit depletion (unproven hypothesis) **Test:** `testPermitLeakOnHttp2GoAwayDoubleQueuedListener` The thread dumps captured during the production deadlock show only 13 threads blocked on `semaphore.acquire()` — far fewer than the 1000 needed to exhaust the semaphore through Phase 2 alone. This means the semaphore must already have been near zero *before* the final burst event, implying a slow background depletion process had been running for some time prior. We have no direct observation of this drain — no metrics were in place to track available permits over time — so this is an inference, not a measured fact. We have no proof of what causes this slow depletion. Several hypotheses are on the table: - **Jetty HTTP/2 GOAWAY race:** when a request is dispatched to a connection already marked closed, `HttpConnectionOverHTTP2.send()` may return `SendFailure(ClosedChannelException, retry=true)`, causing `HttpDestination.process()` to re-enqueue the same exchange and fire `notifyQueued()` a second time. This would call `semaphore.acquire()` twice for a single logical request while `onComplete` fires only once — leaking one permit per event. This is the hypothesis the test targets. But since we run with `-Dsolr.http1=true` this finding should not be relevant to us. - **Idle connection expiry:** two of the 13 stuck threads in the reproduction dump show `HttpConnectionOverHTTP.onIdleExpired()` on the path leading to `LBAsyncSolrClient.doAsyncRequest()`. It is possible that idle connection eviction events trigger a code path that acquires a semaphore permit without a guaranteed corresponding release. Attempts to reproduce this in isolation have been inconclusive. - **Unknown cause:** there may be other conditions in the Jetty client or Solr's use of it that produce unmatched acquires, particularly under the connection churn typical of a Kubernetes environment. The test simulates the double `onRequestQueued` notification directly via reflection and asserts that the permit count is unchanged after a single `onComplete`. It **fails** without a `PERMIT_ACQUIRED_ATTR` idempotency guard — demonstrating that this specific pattern *would* leak permits if it occurs in practice. However, we have not confirmed that Jetty actually fires `notifyQueued` twice under real conditions in our cluster or unit tests trying to provoke this with a mock HTTP server. The guard and its TODO are included here to accompany the Phase 2 fix and to invite review of whether this path is reachable. Regardless of the true cause, the implication is the same: something slowly consumes permits in the background until the semaphore is nearly exhausted, at which point Phase 2 delivers the final blow. ### Phase 2 — Sudden full exhaustion (confirmed: re-entrant retry on the IO selector thread) **Test:** `testSemaphoreLeakOnLBRetry` When a connection-level failure occurs, Jetty fires `onFailure` on the IO selector thread. `HttpJettySolrClient.requestAsync` completes its `CompletableFuture` exceptionally from within that callback — still on the IO thread. `LBAsyncSolrClient.doAsyncRequest` registered a `whenComplete` on that future; because the future completes on the IO thread, `whenComplete` also fires **synchronously on the same IO thread**. The `whenComplete` action calls `doAsyncRequest` again as a retry to the next endpoint, which calls `semaphore.acquire()` — still on the IO thread — **before** the original request's `completeListener` has had a chance to call `semaphore.release()`. If the semaphore is already at zero (from Phase 1 depletion), `acquire()` **blocks the IO thread**. The blocked IO thread can no longer execute the `completeListener` that would release the original permit. The permit
