Skip to content

make maximumConcurrentConnections configurable #373

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

Closed
weissi opened this issue Jun 15, 2021 · 7 comments · Fixed by #437
Closed

make maximumConcurrentConnections configurable #373

weissi opened this issue Jun 15, 2021 · 7 comments · Fixed by #437
Assignees

Comments

@weissi
Copy link
Contributor

weissi commented Jun 15, 2021

  • maximumConcurrentConnections is an important property for any system using AHC, it therefore should really be configurable
  • 8 seems also very low

Technically, the introduction of the "max 8 connections at a time" can be seen as API breaking in AHC version 1.1.0. There are certainly programs that worked correctly with AHC 1.0.0 (without the pool) and then broke with 1.1.0 which limited the number of concurrent connections to 8.

@weissi
Copy link
Contributor Author

weissi commented Jun 15, 2021

cf rdar://79360051

@Lukasa
Copy link
Collaborator

Lukasa commented Jun 15, 2021

Agreed that maximumConcurrentConnections should be configurable. The default of 8 is in the ballpark of typical browsers (which tend to land at 6) and seems like a reasonable default for many use-cases, but it should definitely be overridable.

@weissi
Copy link
Contributor Author

weissi commented Jun 15, 2021

@Lukasa well, isn't the browser limit 6 per host? The AHC limit is AFAIK a global maximum connection limit, ie. AHC will never do more than 8 open connections.

@Lukasa
Copy link
Collaborator

Lukasa commented Jun 15, 2021

Yes, it's 6 per origin. So far as I can see in the AHC code the 8 limit is also per origin: it's in the HTTP1ConnectionProvider, which is one-per-origin.

@weissi
Copy link
Contributor Author

weissi commented Jun 15, 2021

@Lukasa I think you're right, thank you! Okay, in that case let's call 8 a good default but we should make it configurable.

@Lukasa
Copy link
Collaborator

Lukasa commented Jun 15, 2021

@fabianfett As you're doing a substantial rework of the pool, want to factor this in? May be worth making this change once on mainline because it should be straightforward, but making sure the API can fold nicely into your work.

@fabianfett fabianfett self-assigned this Jun 16, 2021
@fabianfett fabianfett added this to the 1.5.0 milestone Jun 18, 2021
@weissi
Copy link
Contributor Author

weissi commented Jun 30, 2021

@fabianfett I think this needs to be moved to the 1.6.0 milestone, correct?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants