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

HTTPClient(s) in the Connector should be pooled #60

Closed
cleemullins opened this issue Feb 5, 2018 · 5 comments
Closed

HTTPClient(s) in the Connector should be pooled #60

cleemullins opened this issue Feb 5, 2018 · 5 comments
Assignees
Milestone

Comments

@cleemullins
Copy link
Contributor

The class EndorsementsRetriever.cs creates a new HttpClient for each request. This is wasteful of resources, and significantly increases latency.

This should be pooled and/or static.

@RyanDawkins
Copy link

Looks like this can be closed.

@cleemullins
Copy link
Contributor Author

The remaining one to fix is the ConnectorClient. It's called in few places.

@EricDahlvang is looking into this now. It's complex because it's down in auto-generated code and the patterns for fixing it are non-obvious.

@EricDahlvang
Copy link
Member

In August, a constructor was introduced to ServiceClient that allowed consuming code to specify if HttpClient should be disposed https://github.com/Azure/azure-sdk-for-net/pull/3547/files . A method was also added that ensures ProductInfoHeaderValues are not added to UserAgent twice. However, this throws exceptions when a ProductInfoHeaderValue is provided that does not have a Product (i.e. constructed with comment only). Also, the ServiceClient itself adds a ProductInfoHeaderValue every time. These things make the HttpClient constructor on ServiceClient an invalid option for re-using a static HttpClient . ( filed this issue: Azure/azure-sdk-for-net#4086 and this PR Azure/azure-sdk-for-net#4095 )

These things considered, the only viable option at this time is something like what @tomlm submitted here: https://github.com/Microsoft/BotBuilder/blob/d09bcc9c46e2a16d9d523f4209387fd0aaa68b29/CSharp/Library/Microsoft.Bot.Connector.Shared/ConnectorClientEx.cs

@cleemullins
Copy link
Contributor Author

With Tom's most recent changes, #186, this is now done.
@EricDahlvang

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

No branches or pull requests

3 participants