You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hc.apache.org by "arturobernalg (via GitHub)" <gi...@apache.org> on 2025/09/27 17:02:04 UTC
[PR] HTTPCLIENT-2398 - Cap async execution queue to break recursive re-entry [httpcomponents-client]
arturobernalg opened a new pull request, #728:
URL: https://github.com/apache/httpcomponents-client/pull/728
Implements Option 1 from the discussion: add a configurable cap on the `async` execution pipeline to prevent the recursive callback loop that led to `StackOverflowError`.
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2398 - Cap async execution queue to break recursive re-entry [httpcomponents-client]
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg merged PR #728:
URL: https://github.com/apache/httpcomponents-client/pull/728
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2398 - Cap async execution queue to break recursive re-entry [httpcomponents-client]
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on PR #728:
URL: https://github.com/apache/httpcomponents-client/pull/728#issuecomment-3610833709
> ks good. Can we add the same functionality to `H2AsyncClientBuilder` as well?
@ok2c please do another pass
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2398 - Cap async execution queue to break recursive re-entry [httpcomponents-client]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on code in PR #728:
URL: https://github.com/apache/httpcomponents-client/pull/728#discussion_r2573560388
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/InternalHttpAsyncExecRuntime.java:
##########
@@ -218,8 +250,7 @@ public Cancellable connectEndpoint(
return Operations.nonCancellable();
}
final RequestConfig requestConfig = context.getRequestConfigOrDefault();
- @SuppressWarnings("deprecation")
- final Timeout connectTimeout = requestConfig.getConnectTimeout();
+ @SuppressWarnings("deprecation") final Timeout connectTimeout = requestConfig.getConnectTimeout();
Review Comment:
@arturobernalg Why? Is this needed?
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2398 - Cap async execution queue to break recursive re-entry [httpcomponents-client]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on PR #728:
URL: https://github.com/apache/httpcomponents-client/pull/728#issuecomment-3591794164
@arturobernalg Could you please rebase this change-set?
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2398 - Cap async execution queue to break recursive re-entry [httpcomponents-client]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on code in PR #728:
URL: https://github.com/apache/httpcomponents-client/pull/728#discussion_r2594736146
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/InternalH2AsyncExecRuntime.java:
##########
@@ -184,35 +206,34 @@ public Cancellable connectEndpoint(
}
final HttpRoute route = endpoint.route;
final RequestConfig requestConfig = context.getRequestConfigOrDefault();
- @SuppressWarnings("deprecation")
- final Timeout connectTimeout = requestConfig.getConnectTimeout();
+ @SuppressWarnings("deprecation") final Timeout connectTimeout = requestConfig.getConnectTimeout();
Review Comment:
@arturobernalg There is again plenty of white space / formatting noise. Please try to avoid it.
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/InternalH2AsyncExecRuntime.java:
##########
@@ -246,10 +267,64 @@ public EndpointInfo getEndpointInfo() {
return null;
}
+ private boolean tryAcquireSlot() {
+ if (sharedQueued == null) {
+ return true;
+ }
+ for (; ; ) {
+ final int q = sharedQueued.get();
+ if (q >= maxQueued) {
+ return false;
+ }
+ if (sharedQueued.compareAndSet(q, q + 1)) {
+ return true;
+ }
+ }
+ }
+
+ private void releaseSlot() {
+ if (sharedQueued != null) {
+ sharedQueued.decrementAndGet();
+ }
+ }
+
+ private AsyncClientExchangeHandler guard(final AsyncClientExchangeHandler handler) {
Review Comment:
@arturobernalg I am not super crazy about code re-use at all cost but in this case there is just too much duplicate code. Please consider extracting into a separate proxy class similar to `RequestEntityProxy` / `ResponseEntityProxy`
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2398 - Cap async execution queue to break recursive re-entry [httpcomponents-client]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on PR #728:
URL: https://github.com/apache/httpcomponents-client/pull/728#issuecomment-3647603158
@arturobernalg This is better. Please mark new setters in the builder classes as experimental.
Okay, this is what we have now
Users could now limit the number of concurrent active requests on a per client basis. How useful is that, though? Say, one sets max concurrent request number to 1000, then opens 100 active connections and equally distributes requests over those connections. 10 concurrent requests per connection is very little. However, having just one connection and trying to execute 1000 requests concurrently over it would likely be wrong.
What would be more useful is to cap the total number of requests on a per connection basis.
What could also be useful is enqueuing requests instead of failing them and executing them as soon as the number of concurrent requests drops below the maximum. Moreover, the same queuing mechanism could be used to re-queue and retry requests that did not get executed due to the opposite endpoint closing the connection.
This change-set is a start in the right direction. Let's keep it though clearly marked as experimental for now.
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2398 - Cap async execution queue to break recursive re-entry [httpcomponents-client]
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on PR #728:
URL: https://github.com/apache/httpcomponents-client/pull/728#issuecomment-3691465171
> > I’ll follow up with separate PRs for (1) per-connection caps
>
>
>
> @arturobernalg Please note this will likely have to be done in core.
@ok2c Ill take a look the option to move it to the core
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2398 - Cap async execution queue to break recursive re-entry [httpcomponents-client]
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on PR #728:
URL: https://github.com/apache/httpcomponents-client/pull/728#issuecomment-3691679293
> > I’ll follow up with separate PRs for (1) per-connection caps
>
> @arturobernalg Please note this will likely have to be done in core.
@ok2c I replaced the `AsyncClientExchangeHandler` dynamic proxy with a simple delegating wrapper so we don’t pay reflection overhead on the hot path; behavior stays the same, with the slot released exactly once on `releaseResources()`. Please do another pass
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2398 - Cap async execution queue to break recursive re-entry [httpcomponents-client]
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on PR #728:
URL: https://github.com/apache/httpcomponents-client/pull/728#issuecomment-3621892172
@ok2c got it. No more static var.
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2398 - Cap async execution queue to break recursive re-entry [httpcomponents-client]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on code in PR #728:
URL: https://github.com/apache/httpcomponents-client/pull/728#discussion_r2573560647
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/InternalHttpAsyncExecRuntime.java:
##########
@@ -241,7 +272,7 @@ public void completed(final AsyncConnectionEndpoint endpoint) {
}
}
- }));
+ }));
Review Comment:
@arturobernalg Is this needed? Formatting noise?
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2398 - Cap async execution queue to break recursive re-entry [httpcomponents-client]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on PR #728:
URL: https://github.com/apache/httpcomponents-client/pull/728#issuecomment-3691456889
> I’ll follow up with separate PRs for (1) per-connection caps
@arturobernalg Please note this will likely have to be done in core.
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2398 - Cap async execution queue to break recursive re-entry [httpcomponents-client]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on code in PR #728:
URL: https://github.com/apache/httpcomponents-client/pull/728#discussion_r2573564805
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/InternalHttpAsyncExecRuntime.java:
##########
@@ -282,10 +313,115 @@ public EndpointInfo getEndpointInfo() {
return endpoint != null ? endpoint.getInfo() : null;
}
+ private boolean tryAcquireSlot() {
+ if (sharedQueued == null) {
+ return true;
+ }
+ for (; ; ) {
+ final int q = sharedQueued.get();
+ if (q >= maxQueued) {
+ return false;
+ }
+ if (sharedQueued.compareAndSet(q, q + 1)) {
+ return true;
+ }
+ }
+ }
+
+ private void releaseSlot() {
+ if (sharedQueued != null) {
+ sharedQueued.decrementAndGet();
+ }
+ }
+
+ private AsyncClientExchangeHandler guard(final AsyncClientExchangeHandler h) {
+ return new AsyncClientExchangeHandler() {
+ private final AtomicBoolean done = new AtomicBoolean(false);
+
+ private void finish() {
+ if (done.compareAndSet(false, true)) {
+ releaseSlot();
+ }
+ }
+
+ @Override
+ public void failed(final Exception cause) {
+ try {
+ h.failed(cause);
+ } finally {
+ finish();
+ }
+ }
+
+ @Override
+ public void cancel() {
+ try {
+ h.cancel();
+ } finally {
+ finish();
+ }
+ }
+
+ @Override
+ public void releaseResources() {
Review Comment:
@arturobernalg I imagine calling `#finish` in `#releaseResources` only should be enough and probably more semantically correct.
Can we also avoid wrapping each and every `AsyncClientExchangeHandler` instead? Can we do it only when `sharedQueued` is being used? By the way, you may want to use dynamic proxy here in order to avoid having to override every method in the interface.
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2398 - Cap async execution queue to break recursive re-entry [httpcomponents-client]
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on code in PR #728:
URL: https://github.com/apache/httpcomponents-client/pull/728#discussion_r2596055493
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/InternalHttpAsyncExecRuntime.java:
##########
@@ -66,6 +70,9 @@ static class ReUseData {
}
+ private static final ConcurrentMap<AsyncClientConnectionManager, AtomicInteger> QUEUE_COUNTERS =
Review Comment:
@ok2c please do another pass
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2398 - Cap async execution queue to break recursive re-entry [httpcomponents-client]
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on code in PR #728:
URL: https://github.com/apache/httpcomponents-client/pull/728#discussion_r2576566283
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/InternalHttpAsyncExecRuntime.java:
##########
@@ -282,10 +313,115 @@ public EndpointInfo getEndpointInfo() {
return endpoint != null ? endpoint.getInfo() : null;
}
+ private boolean tryAcquireSlot() {
+ if (sharedQueued == null) {
+ return true;
+ }
+ for (; ; ) {
+ final int q = sharedQueued.get();
+ if (q >= maxQueued) {
+ return false;
+ }
+ if (sharedQueued.compareAndSet(q, q + 1)) {
+ return true;
+ }
+ }
+ }
+
+ private void releaseSlot() {
+ if (sharedQueued != null) {
+ sharedQueued.decrementAndGet();
+ }
+ }
+
+ private AsyncClientExchangeHandler guard(final AsyncClientExchangeHandler h) {
+ return new AsyncClientExchangeHandler() {
+ private final AtomicBoolean done = new AtomicBoolean(false);
+
+ private void finish() {
+ if (done.compareAndSet(false, true)) {
+ releaseSlot();
+ }
+ }
+
+ @Override
+ public void failed(final Exception cause) {
+ try {
+ h.failed(cause);
+ } finally {
+ finish();
+ }
+ }
+
+ @Override
+ public void cancel() {
+ try {
+ h.cancel();
+ } finally {
+ finish();
+ }
+ }
+
+ @Override
+ public void releaseResources() {
Review Comment:
@ok2c please do another pass
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2398 - Cap async execution queue to break recursive re-entry [httpcomponents-client]
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on code in PR #728:
URL: https://github.com/apache/httpcomponents-client/pull/728#discussion_r2647247592
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/ReleasingAsyncClientExchangeHandler.java:
##########
@@ -0,0 +1,122 @@
+/*
+ * ====================================================================
+ * 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.
+ * ====================================================================
+ *
+ * This software consists of voluntary contributions made by many
+ * individuals on behalf of the Apache Software Foundation. For more
+ * information on the Apache Software Foundation, please see
+ * <http://www.apache.org/>.
+ *
+ */
+package org.apache.hc.client5.http.impl.async;
+
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import org.apache.hc.core5.annotation.Internal;
+import org.apache.hc.core5.http.EntityDetails;
+import org.apache.hc.core5.http.Header;
+import org.apache.hc.core5.http.HttpException;
+import org.apache.hc.core5.http.HttpResponse;
+import org.apache.hc.core5.http.nio.AsyncClientExchangeHandler;
+import org.apache.hc.core5.http.nio.CapacityChannel;
+import org.apache.hc.core5.http.nio.DataStreamChannel;
+import org.apache.hc.core5.http.nio.RequestChannel;
+import org.apache.hc.core5.http.protocol.HttpContext;
+
+@Internal
+final class ReleasingAsyncClientExchangeHandler implements AsyncClientExchangeHandler {
+
+ private final AsyncClientExchangeHandler handler;
+ private final Runnable onRelease;
+ private final AtomicBoolean released;
+
+ ReleasingAsyncClientExchangeHandler(final AsyncClientExchangeHandler handler, final Runnable onRelease) {
+ this.handler = handler;
+ this.onRelease = onRelease;
+ this.released = new AtomicBoolean(false);
+ }
+
+ @Override
+ public void produceRequest(final RequestChannel channel, final HttpContext context) throws HttpException, IOException {
+ handler.produceRequest(channel, context);
+ }
+
+ @Override
+ public void consumeResponse(final HttpResponse response, final EntityDetails entityDetails, final HttpContext context)
+ throws HttpException, IOException {
+ handler.consumeResponse(response, entityDetails, context);
+ }
+
+ @Override
+ public void consumeInformation(final HttpResponse response, final HttpContext context) throws HttpException, IOException {
+ handler.consumeInformation(response, context);
+ }
+
+ @Override
+ public int available() {
+ return handler.available();
+ }
+
+ @Override
+ public void produce(final DataStreamChannel channel) throws IOException {
+ handler.produce(channel);
+ }
+
+ @Override
+ public void updateCapacity(final CapacityChannel capacityChannel) throws IOException {
+ handler.updateCapacity(capacityChannel);
+ }
+
+ @Override
+ public void consume(final ByteBuffer src) throws IOException {
+ handler.consume(src);
+ }
+
+ @Override
+ public void streamEnd(final List<? extends Header> trailers) throws HttpException, IOException {
+ handler.streamEnd(trailers);
+ }
+
+ @Override
+ public void failed(final Exception cause) {
+ handler.failed(cause);
+ }
+
+ @Override
+ public void cancel() {
+ handler.cancel();
+ }
+
+ @Override
+ public void releaseResources() {
+ try {
+ handler.releaseResources();
+ } finally {
+ if (released.compareAndSet(false, true)) {
+ try {
+ onRelease.run();
+ } catch (final RuntimeException ignore) {
Review Comment:
> @arturobernalg Why do need to catch and ignore all `RuntimeException`s here? This does not look quite right to me. Why not let it propagate?
@ok2c Agreed. There’s no good reason to swallow `RuntimeException` here; onRelease is an internal decrement callback and should not throw. I removed the catch and keep it guarded by the CAS in the finally block.
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2398 - Cap async execution queue to break recursive re-entry [httpcomponents-client]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on code in PR #728:
URL: https://github.com/apache/httpcomponents-client/pull/728#discussion_r2595253079
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/InternalHttpAsyncExecRuntime.java:
##########
@@ -66,6 +70,9 @@ static class ReUseData {
}
+ private static final ConcurrentMap<AsyncClientConnectionManager, AtomicInteger> QUEUE_COUNTERS =
Review Comment:
@arturobernalg This is bad. We need to find a way to maintain the counter without using a static hash map. The limit should ideally be per connection. If this is too difficult, then, per client instance.
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2398 - Cap async execution queue to break recursive re-entry [httpcomponents-client]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on code in PR #728:
URL: https://github.com/apache/httpcomponents-client/pull/728#discussion_r2596200761
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/InternalHttpAsyncExecRuntime.java:
##########
@@ -66,6 +70,9 @@ static class ReUseData {
}
+ private static final ConcurrentMap<AsyncClientConnectionManager, AtomicInteger> QUEUE_COUNTERS =
Review Comment:
@arturobernalg Do not see how this is any better. The static map is still there and what you have is essentially a major resource leak right there.
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2398 - Cap async execution queue to break recursive re-entry [httpcomponents-client]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on PR #728:
URL: https://github.com/apache/httpcomponents-client/pull/728#issuecomment-3608144392
@arturobernalg Looks good. Can we add the same functionality to `H2AsyncClientBuilder` 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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2398 - Cap async execution queue to break recursive re-entry [httpcomponents-client]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on code in PR #728:
URL: https://github.com/apache/httpcomponents-client/pull/728#discussion_r2573559942
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/InternalHttpAsyncExecRuntime.java:
##########
@@ -59,13 +75,15 @@ static class ReUseData {
final Object state;
final TimeValue validDuration;
- ReUseData(final Object state, final TimeValue validDuration) {
- this.state = state;
- this.validDuration = validDuration;
- }
+ ReUseData(final Object state, final TimeValue validDuration) {
Review Comment:
@arturobernalg tabs?
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2398 - Cap async execution queue to break recursive re-entry [httpcomponents-client]
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on PR #728:
URL: https://github.com/apache/httpcomponents-client/pull/728#issuecomment-3689012663
> What could also be useful is enqueuing requests instead of failing them and executing them as soon as the number of concurrent requests drops below the maximum. Moreover, the same queuing mechanism could be used to re-queue and retry requests that did not get executed due to the opposite endpoint closing the connection.
@ok2c Marked the new builder setters as @Experimental. Agreed on the limitations of a per-client cap; I’ll keep this change-set scoped to fail-fast limiting for now. I’ll follow up with separate PRs for (1) per-connection caps and (2) optional bounded queuing / re-queue of not-yet-started requests.
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2398 - Cap async execution queue to break recursive re-entry [httpcomponents-client]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on code in PR #728:
URL: https://github.com/apache/httpcomponents-client/pull/728#discussion_r2647229161
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/ReleasingAsyncClientExchangeHandler.java:
##########
@@ -0,0 +1,122 @@
+/*
+ * ====================================================================
+ * 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.
+ * ====================================================================
+ *
+ * This software consists of voluntary contributions made by many
+ * individuals on behalf of the Apache Software Foundation. For more
+ * information on the Apache Software Foundation, please see
+ * <http://www.apache.org/>.
+ *
+ */
+package org.apache.hc.client5.http.impl.async;
+
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import org.apache.hc.core5.annotation.Internal;
+import org.apache.hc.core5.http.EntityDetails;
+import org.apache.hc.core5.http.Header;
+import org.apache.hc.core5.http.HttpException;
+import org.apache.hc.core5.http.HttpResponse;
+import org.apache.hc.core5.http.nio.AsyncClientExchangeHandler;
+import org.apache.hc.core5.http.nio.CapacityChannel;
+import org.apache.hc.core5.http.nio.DataStreamChannel;
+import org.apache.hc.core5.http.nio.RequestChannel;
+import org.apache.hc.core5.http.protocol.HttpContext;
+
+@Internal
+final class ReleasingAsyncClientExchangeHandler implements AsyncClientExchangeHandler {
+
+ private final AsyncClientExchangeHandler handler;
+ private final Runnable onRelease;
+ private final AtomicBoolean released;
+
+ ReleasingAsyncClientExchangeHandler(final AsyncClientExchangeHandler handler, final Runnable onRelease) {
+ this.handler = handler;
+ this.onRelease = onRelease;
+ this.released = new AtomicBoolean(false);
+ }
+
+ @Override
+ public void produceRequest(final RequestChannel channel, final HttpContext context) throws HttpException, IOException {
+ handler.produceRequest(channel, context);
+ }
+
+ @Override
+ public void consumeResponse(final HttpResponse response, final EntityDetails entityDetails, final HttpContext context)
+ throws HttpException, IOException {
+ handler.consumeResponse(response, entityDetails, context);
+ }
+
+ @Override
+ public void consumeInformation(final HttpResponse response, final HttpContext context) throws HttpException, IOException {
+ handler.consumeInformation(response, context);
+ }
+
+ @Override
+ public int available() {
+ return handler.available();
+ }
+
+ @Override
+ public void produce(final DataStreamChannel channel) throws IOException {
+ handler.produce(channel);
+ }
+
+ @Override
+ public void updateCapacity(final CapacityChannel capacityChannel) throws IOException {
+ handler.updateCapacity(capacityChannel);
+ }
+
+ @Override
+ public void consume(final ByteBuffer src) throws IOException {
+ handler.consume(src);
+ }
+
+ @Override
+ public void streamEnd(final List<? extends Header> trailers) throws HttpException, IOException {
+ handler.streamEnd(trailers);
+ }
+
+ @Override
+ public void failed(final Exception cause) {
+ handler.failed(cause);
+ }
+
+ @Override
+ public void cancel() {
+ handler.cancel();
+ }
+
+ @Override
+ public void releaseResources() {
+ try {
+ handler.releaseResources();
+ } finally {
+ if (released.compareAndSet(false, true)) {
+ try {
+ onRelease.run();
+ } catch (final RuntimeException ignore) {
Review Comment:
@arturobernalg Why do need to catch and ignore all `RuntimeException`s here? This does not look quite right to me. Why not let it propagate?
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org