Skip to content
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

New HTTP client connection pooling #8100

Merged
merged 86 commits into from
Oct 19, 2022
Merged

New HTTP client connection pooling #8100

merged 86 commits into from
Oct 19, 2022

Conversation

yawkat
Copy link
Member

@yawkat yawkat commented Sep 30, 2022

This PR adds a new connection pooling infrastructure to the HTTP client. The biggest changes are in ConnectionManager, which contains network setup code, and in the new PoolResizer class, which handles pool dynamics (it calls into ConnectionManager.Pool to assign requests, establish new connections, etc, but does no network setup by itself).

Changes in no particular order:

  • The http-version config setting has been replaced by two settings. plaintext-mode applies to http URLs and can be set to either HTTP_1 or H2C. alpn-modes applies to https URLs, and determines the protocols that should be supported in protocol negotiation (it's a list, with the supported values h2 and http/1.1). The old http-version setting remains for compatibility, and should remain for 4.0. It is also used in some test cases.
  • connection pooling now applies to all connections (except websockets), not just exchange calls, and it is enabled by default.
  • HTTP2 connections now use a channel-per-stream model (like One channel per HTTP2 stream #6842), and allow concurrent requests on one connection.
  • Connection pool configuration is more fine-grained, with separate settings for HTTP1 and HTTP2.
  • Pooled connections are now tracked using netty's resource leak detection, to avoid "dangling" connections in the pool (note: resource leak detection is still only tested widely for the tests in the http-server-netty module atm).
  • One minor fix in the http server that came up in the test suite because connection pooling is on by default now.

There are still some TODOs in this PR. Some relate to pending netty improvements (netty/netty#12827 , netty/netty#12830), but work fine for the moment. Some are future improvements that could be made, but that I want to do in separate PRs.

@yawkat
Copy link
Member Author

yawkat commented Sep 30, 2022

that would make sense. that's a task for future me :)

@yawkat yawkat self-assigned this Sep 30, 2022
@yawkat
Copy link
Member Author

yawkat commented Oct 10, 2022

@timyates WDYT

# Conflicts:
#	http-server-netty/src/test/groovy/io/micronaut/http/server/netty/binding/HttpResponseSpec.groovy
# Conflicts:
#	http-client/src/main/java/io/micronaut/http/client/netty/DefaultHttpClient.java
#	http-server-netty/src/main/java/io/micronaut/http/server/netty/RoutingInBoundHandler.java
@dstepanov
Copy link
Contributor

Would it be possible to refactor DefaultHttpClient to be created using the builder? I just had to add one additional parameter to the constructor and its pain.

@yawkat
Copy link
Member Author

yawkat commented Oct 13, 2022

it would definitely be an improvement

Comment on lines -688 to -705
/**
* The maximum number of connections. Defaults to ({@value io.micronaut.http.client.HttpClientConfiguration.ConnectionPoolConfiguration#DEFAULT_MAXCONNECTIONS}); no maximum.
*
* @return The max connections
*/
public int getMaxConnections() {
return maxConnections;
}

/**
* Sets the maximum number of connections. Defaults to no maximum.
*
* @param maxConnections The count
*/
public void setMaxConnections(int maxConnections) {
this.maxConnections = maxConnections;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we deprecate instead of remove?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure but why, since it's going into 4.0 anyway?

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

86.3% 86.3% Coverage
0.0% 0.0% Duplication

@yawkat
Copy link
Member Author

yawkat commented Oct 19, 2022

@graemerocher can you look at this some time this week so that we can merge it before my vacation, @timyates wants to do some work moving the http modules out of micronaut-core, and that is blocked by this PR

@yawkat yawkat merged commit 0f34209 into 4.0.x Oct 19, 2022
@yawkat yawkat deleted the request-channel-4x branch October 19, 2022 16:01
@yawkat
Copy link
Member Author

yawkat commented Oct 20, 2022

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants