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

maxConnections setting is not respected #880

Closed
aajtodd opened this issue Jun 29, 2023 · 1 comment · Fixed by #904
Closed

maxConnections setting is not respected #880

aajtodd opened this issue Jun 29, 2023 · 1 comment · Fixed by #904
Assignees
Labels
bug This issue is a bug.

Comments

@aajtodd
Copy link
Contributor

aajtodd commented Jun 29, 2023

Setting maxConnections or maxConnectionsPerHost doesn't actually limit the number of connections in the okhttp engine. This is likely to cause confusion at best or much worse for teams that actually need hard limits on these resources.

This setting is also inaccurately mapped in the CRT engine since we create a connection manager per host with maxConnections setting. This means the CRT engine may also not respect total maxConnections depending on number of hosts/load.

For OkHttp we should investigate adding a custom gate/limiter that stalls the request and waits for a connection if one isn't available yet. This may require signaling from an event/connection listener to know when connections are released. This would allow us to support max connection settings for okhttp still. For CRT we may need something similar.

@aajtodd aajtodd added the bug This issue is a bug. label Jun 29, 2023
@aajtodd aajtodd changed the title OkHttp maxConnections setting is not respected maxConnections setting is not respected Jul 19, 2023
@aajtodd
Copy link
Contributor Author

aajtodd commented Jul 24, 2023

Fixing/enforcing this in OkHttp proved to be difficult. Instead we've decided to relocate maxConnections out of the generic HTTP engine config and allow specific engines to expose this setting if they can meaningfully enforce it. We may revisit this in the future for OkHttp if we find a way to enforce it without detrimental secondary effects on throughput.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug.
Projects
None yet
1 participant