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.

Sign up for GitHub

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

Jump to bottom

HttpClient disposal anti-pattern #106

Closed
drub0y opened this issue Feb 14, 2018 · 8 comments
Closed

HttpClient disposal anti-pattern #106

drub0y opened this issue Feb 14, 2018 · 8 comments
Assignees
@cleemullins
Labels
bug Indicates an unexpected problem or an unintended behavior.

Comments

@drub0y
Copy link
Contributor

drub0y commented Feb 14, 2018

There are several instances of the HttpClient disposal anti-pattern within the code base. Under a sufficient amount of load this will cause socket level resource exhaustion that will result in intermittent connection failures and, ultimately, prevent bots from scaling.

The following classes are currently using this anti-pattern:

  • Microsoft.Bot.Builder.Ai
    • Translator
  • Microsoft.Bot.Builder.Connector
    • EndorsementsRetriever
    • MicrosoftAppCredentials
    • AttachmentsEx
@drub0y drub0y mentioned this issue Feb 14, 2018
Closed
@cleemullins cleemullins self-assigned this Feb 14, 2018
@cleemullins cleemullins added the bug Indicates an unexpected problem or an unintended behavior. label Feb 14, 2018
@cleemullins
Copy link
Contributor

I have most of these tagged for looking at. Thanks for driving home that I need to fix them sooner rather than later.

+1

@RyanDawkins
Copy link

I've made a start at this and for #107 on my fork.

I've started on this for the QnAMaker and the Translator class.

RyanDawkins@83a5122

I have implemented the changes for the Translator and the QnA maker and I'm in the process of testing.

@cleemullins
Copy link
Contributor

Thanks Ryan! I was actually just reading about best practices around HttpClient and will look forward to seeing your PR.

@RyanDawkins
Copy link

Just tested the QnA version! I'm about to setup Luis to make sure that is working too.

@cleemullins
Copy link
Contributor

cleemullins commented Feb 15, 2018 via email

@RyanDawkins
Copy link

Here you go!
#122

@EricDahlvang EricDahlvang mentioned this issue Feb 15, 2018
Closed
@cleemullins cleemullins mentioned this issue Feb 15, 2018
Merged
@cleemullins
Copy link
Contributor

Pending PR for the remaining places this code existed: #133

@cleemullins
Copy link
Contributor

Fixed. Thanks.

ceciliaavila pushed a commit that referenced this issue Jun 25, 2019
@matiasroldan6
Slack Adapter polish (#106)
30fcf87 
* Remove unused formatter, remove duplicate response code set, remove unnecessary casts

* Add Slack login correctly done

* Fix id assignation in SendActivitiesAsync method (#107)

* Add some minor stylecop corrections
LoginWithSlack method temporarily waited to test

* Fix minor stylecop warnings, fix response sending, remove unused inclusion, start partial implementation of previously unimplemented ChannelData message assignment, add property to NewSlackMessage class

* Remove ChatPostEphemeralMessageResult and ChatPostMessageResult classes

* Fix message.ThreadTS assignment and cast. Complete message ChannelData assignent. Some stylecop corrections

* Add stylecop corrections, not taking into account parameter documentations

* Change the return type of ActivityToSlack

* Remove references to Middleware

* Remove Middleware files

* Fix thread_ts assignation

* Add PR changes

* Remove commented out lines

* Add PR changes: commented out code removed, some dynamic casts removed

* Remove all dynamics cast to slackEvent

* Add more PR changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@cleemullins cleemullins

Labels
bug Indicates an unexpected problem or an unintended behavior.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@drub0y @cleemullins @RyanDawkins