Cap total number of concurrent requests per HTTP/2 connection #592
Conversation
|
@arturobernalg I do not like this approach. There is already a command queue built into the |
arturobernalg
force-pushed
the
max-requests-per-connection
branch
from
de6f206 to
b777f5d
Compare
@ok2c . Sorry about the churn — I’m still getting up to speed on this part of the reactor stack and my previous commit took a wrong turn. I dropped the extra wrapper and enforce the cap via the existing |
| * @since 5.5 | ||
| */ | ||
| default int getPendingCommandCount() { | ||
| return 0; |
There was a problem hiding this comment.
@arturobernalg I think the method by default return -1 as not known
| * @return {@code true} if the command was enqueued, {@code false} otherwise. | ||
| * @since 5.5 | ||
| */ | ||
| default boolean enqueue(final Command command, final Command.Priority priority, final int maxPendingCommands) { |
There was a problem hiding this comment.
@arturobernalg I would not do that on the i/o session level. Different protocol handlers may have different restrictions. 10 concurrent requests may be perfectly OK for HTTP/2 and too much for HTTP/1.1. Please move this logic to individual protocol handlers.
There was a problem hiding this comment.
@ok2c I just drop the IOSession-level enqueue/queue-limit API and keep the cap/reject logic strictly in the HTTP/2 requester/handler layer as requested.
arturobernalg
force-pushed
the
max-requests-per-connection
branch
from
3cbecbe to
639e632
Compare
|
|
||
| @Override | ||
| public void releaseResources() { | ||
| final int max = maxRequestsPerConnection; |
There was a problem hiding this comment.
@arturobernalg Why, why all this complexity? You are not going to impress no young ladies with it. You almost had it right last time. Restore the command count method from the previous revision and fail the request if the total number of pending commands is over the max limit. That should be all. The tricky bit is to come up with a reasonable config mechanism for the max pending command limit.
There was a problem hiding this comment.
@arturobernalg Why, why all this complexity? You are not going to impress no young ladies with it. You almost had it right last time. Restore the command count method from the previous revision and fail the request if the total number of pending commands is over the max limit. That should be all. The tricky bit is to come up with a reasonable config mechanism for the max pending command limit.
Hi @ok2c , I’ve ditched the extra plumbing and went back to the simple approach: restored the command-count method and now H2MultiplexingRequester just fails fast when pending commands exceed the configured max.
Restore per-session command count and use it in H2MultiplexingRequester to fail fast when the per-connection command queue exceeds the configured limit.
arturobernalg
force-pushed
the
max-requests-per-connection
branch
from
9c8911c to
e6cb499
Compare
| /** | ||
| * Cancellable that can be wired to the stream control once it becomes available. | ||
| */ | ||
| private static final class CancellableExecution implements Cancellable, CancellableDependency { |
There was a problem hiding this comment.
@arturobernalg I am not sure why this is necessary. The original CancellableExecution is now unused. Is this an optimization of some sort?
ok2c
left a comment
ok2c
left a comment
There was a problem hiding this comment.
@arturobernalg Conceptually everything looks good to me. However as far as I can see you are also mixing in some code optimization into the same change-set.
Could you please do this?
- Spin the optimization code into a separate pull request. It should get merged in first.
- Re-base this pull request off it
- Provide the same logic to
HttpAsyncRequesterfor consistency.
2 participants
This change adds an optional hard cap on the number of pending request execution commands queued per HTTP/2 connection. When the per-connection limit is reached, new submissions fail fast with RejectedExecutionException and the exchange handler releases its resources immediately. The cap is configured via H2MultiplexingRequesterBootstrap and passed into H2MultiplexingRequester at construction time to keep requester configuration immutable and avoid API incompatibilities.