Fixes #13261 - Improve handling of failed HTTP/2 requests. #13267
+373
−204
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…Connection, and HttpConnectionOver*. Signed-off-by: Simone Bordet <[email protected]>
Signed-off-by: Simone Bordet <[email protected]>
…t in HttpSender to ensure that aborts always complete the CompletableFuture. Signed-off-by: Simone Bordet <[email protected]>
Signed-off-by: Simone Bordet <[email protected]>
* Reimplemented HttpConnectionOverHTTP2 using locking rather than concurrent data structures. This allows to control when send() is possible or not, with respect to the connection being closed or failed. * Reimplemented HttpConnectionOverHTTP2 close() to call the low layer, while failures are called from the low layer. This allows to have the same code path, as it is always the low layer that drives. * Reimplemented and simplified HTTP2Session failures of streams. Now failure paths properly use Callbacks, rather Callback.NOOP and sequential calls. Signed-off-by: Simone Bordet <[email protected]>
sbordet
added
Sponsored
…f HTTP/2 stream failures. Signed-off-by: Simone Bordet <[email protected]>
gregw
approved these changes
Jun 22, 2025
| @@ -388,8 +388,7 @@ private boolean process(Connection connection) | |||
| LOG.debug("Processing exchange {} on {} of {}", exchange, connection, this); | |||
| if (exchange == null) | |||
| { | |||
| if (!connectionPool.release(connection)) | |||
| connection.close(); | |||
| releaseConnection(connection); | |||
There was a problem hiding this comment.
The else clause below is not necessary and makes this code a little harder to read. Without it, it would be 4 if statements, each returning.
But this is unrelated to this PR
| connection.close(); | ||
| } | ||
| } | ||
|
|
||
| private boolean releaseConnection(Connection connection) |
There was a problem hiding this comment.
Two things confusing about this method on first glance:\
- how is
releaseConnection(Connection)different toreleaseConnection(Connection)? Some better names and/or javadoc would help. - why is the connection closed if it is not released? If that is an unexpected situation, then a warn or debug would be good? A comment in the code to explain would be good. I'm assuming that a connection can be acquired multiple times due to multiplexing, so releasing to non-zero should be normal? or does the boolean return mean something else? Oh the boolean return from the release is special.
Maybe call this releaseOrClose(Connection), because either the connection is released or it is closed?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.