HTTPCLIENT-2398 - Cap async execution queue to break recursive re-entry #728
Conversation
|
@arturobernalg Could you please rebase this change-set? |
arturobernalg
force-pushed
the
HTTPCLIENT-2398-1
branch
from
35f4730 to
d33aa28
Compare
...lient5/src/main/java/org/apache/hc/client5/http/impl/async/InternalHttpAsyncExecRuntime.java
Outdated
Show resolved
Hide resolved
...lient5/src/main/java/org/apache/hc/client5/http/impl/async/InternalHttpAsyncExecRuntime.java
Outdated
Show resolved
Hide resolved
...lient5/src/main/java/org/apache/hc/client5/http/impl/async/InternalHttpAsyncExecRuntime.java
Outdated
Show resolved
Hide resolved
ok2c
left a comment
ok2c
left a comment
There was a problem hiding this comment.
@arturobernalg This is a good start. Please also do not forget about InternalH2AsyncExecRuntime.
...lient5/src/main/java/org/apache/hc/client5/http/impl/async/InternalHttpAsyncExecRuntime.java
Outdated
Show resolved
Hide resolved
arturobernalg
force-pushed
the
HTTPCLIENT-2398-1
branch
from
ce23427 to
a6a1eec
Compare
arturobernalg
force-pushed
the
HTTPCLIENT-2398-1
branch
2 times, most recently
from
0996102 to
9b9e1d4
Compare
|
@arturobernalg Looks good. Can we add the same functionality to |
arturobernalg
force-pushed
the
HTTPCLIENT-2398-1
branch
from
d705c34 to
3bab080
Compare
@ok2c please do another pass |
| final RequestConfig requestConfig = context.getRequestConfigOrDefault(); | ||
| @SuppressWarnings("deprecation") | ||
| final Timeout connectTimeout = requestConfig.getConnectTimeout(); | ||
| @SuppressWarnings("deprecation") final Timeout connectTimeout = requestConfig.getConnectTimeout(); |
There was a problem hiding this comment.
@arturobernalg There is again plenty of white space / formatting noise. Please try to avoid it.
| } | ||
| } | ||
|
|
||
| private AsyncClientExchangeHandler guard(final AsyncClientExchangeHandler handler) { |
There was a problem hiding this 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
arturobernalg
force-pushed
the
HTTPCLIENT-2398-1
branch
from
b7e1ad4 to
111d293
Compare
|
|
||
| } | ||
|
|
||
| private static final ConcurrentMap<AsyncClientConnectionManager, AtomicInteger> QUEUE_COUNTERS = |
There was a problem hiding this 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.
There was a problem hiding this 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.
arturobernalg
force-pushed
the
HTTPCLIENT-2398-1
branch
2 times, most recently
from
6291a62 to
707f4cb
Compare
|
|
||
| } | ||
|
|
||
| private static final ConcurrentMap<AsyncClientConnectionManager, AtomicInteger> QUEUE_COUNTERS = |
There was a problem hiding this 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.
arturobernalg
force-pushed
the
HTTPCLIENT-2398-1
branch
2 times, most recently
from
07a3647 to
1272d81
Compare
|
@ok2c got it. No more static var. |
arturobernalg
force-pushed
the
HTTPCLIENT-2398-1
branch
from
3be5297 to
f4aceac
Compare
…ry. Add configurable maxQueuedRequests (default unlimited). Release slot on fail/cancel/close to avoid leaks
arturobernalg
force-pushed
the
HTTPCLIENT-2398-1
branch
from
ad57ea3 to
319e3d8
Compare
2 participants
Implements Option 1 from the discussion: add a configurable cap on the
asyncexecution pipeline to prevent the recursive callback loop that led toStackOverflowError.