QnAMaker HttpClient disposal anti-pattern #107
Assignees
Labels
bug
Indicates an unexpected problem or an unintended behavior.
Comments
cleemullins
added
the
bug
|
Thanks. I'll take a look at this shortly. |
This was referenced Feb 15, 2018
|
The PR to fix this is pending review. I took the simple approach of just making it static to the QnA Maker component. I Need to do this same thing for other components next... |
|
This is closed out now. |
ceciliaavila
added a commit
that referenced
this issue
Jun 25, 2019
ceciliaavila
pushed a commit
that referenced
this issue
Jun 25, 2019
* 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
ceciliaavila
pushed a commit
that referenced
this issue
Jun 25, 2019
…apper (#115) * 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 * Fix stylecop on the BotKitConversationState file * Fix stylecop on the BotKitDialogWrapper file * Apply PR feedback * Remove unnecessary comment * Fix stylecop on the BotKitConversationState file * Fix stylecop on the BotKitDialogWrapper file * Apply PR feedback * Remove unnecessary comment * Add usings inside the namespace * Fix usings and headers * Fix comment description
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
I've already filed #106 regarding some issues regarding the
HttpClientdisposal anti-pattern, butQnAMakeris a little more nuanced. TheQnAMakerclass at least holds onto itsHttpClientinstance in a field and will reuse it across calls to an instance, but should someone dispose of theQnAMakerinstance it will also dispose of that underlyingHttpClient. This could lead to developers to using theQnAMakerclass in a non-singleton style and thus lead to the same resource starvation issues as mentioned in #106.Recommend moving the
HttpClientinto astaticfield in the class and removing theIDisposableimplementation altogether.The text was updated successfully, but these errors were encountered: