-
Notifications
You must be signed in to change notification settings - Fork 355
Cap total number of concurrent commands per connection #592
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cap total number of concurrent commands per connection #592
Conversation
|
@arturobernalg I do not like this approach. There is already a command queue built into the |
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 |
httpcore5/src/main/java/org/apache/hc/core5/reactor/IOSession.java
Outdated
Show resolved
Hide resolved
httpcore5/src/main/java/org/apache/hc/core5/reactor/IOSession.java
Outdated
Show resolved
Hide resolved
3cbecbe to
639e632
Compare
...5-h2/src/main/java/org/apache/hc/core5/http2/impl/nio/bootstrap/H2MultiplexingRequester.java
Outdated
Show resolved
Hide resolved
9c8911c to
e6cb499
Compare
...5-h2/src/main/java/org/apache/hc/core5/http2/impl/nio/bootstrap/H2MultiplexingRequester.java
Outdated
Show resolved
Hide resolved
ok2c
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
130dedf to
8aa41be
Compare
@ok2c Please another pass |
ok2c
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@arturobernalg Almost there. Just a few comments.
...5-h2/src/main/java/org/apache/hc/core5/http2/impl/nio/bootstrap/H2MultiplexingRequester.java
Outdated
Show resolved
Hide resolved
...5-h2/src/main/java/org/apache/hc/core5/http2/impl/nio/bootstrap/H2MultiplexingRequester.java
Outdated
Show resolved
Hide resolved
...main/java/org/apache/hc/core5/http2/impl/nio/bootstrap/H2MultiplexingRequesterBootstrap.java
Outdated
Show resolved
Hide resolved
...src/test/java/org/apache/hc/core5/http2/examples/H2MaxRequestsPerConnectionLocalExample.java
Outdated
Show resolved
Hide resolved
7ddd29a to
756d5a4
Compare
httpcore5/src/test/java/org/apache/hc/core5/reactor/TestIOSessionImplMaxPendingCommands.java
Outdated
Show resolved
Hide resolved
186431e to
22d2ea5
Compare
...e/hc/core5/http2/impl/nio/bootstrap/TestH2MultiplexingRequesterMaxRequestsPerConnection.java
Outdated
Show resolved
Hide resolved
Restore per-session command count and use it in H2MultiplexingRequester to fail fast when the per-connection command queue exceeds the configured limit.
ecaa298 to
6294a25
Compare
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.