diff --git a/libraries/Microsoft.Bot.Builder/BotFrameworkAdapter.cs b/libraries/Microsoft.Bot.Builder/BotFrameworkAdapter.cs index 6d1a138cc4..94cef42fd4 100644 --- a/libraries/Microsoft.Bot.Builder/BotFrameworkAdapter.cs +++ b/libraries/Microsoft.Bot.Builder/BotFrameworkAdapter.cs @@ -751,9 +751,11 @@ public async Task GetConversationsAsync(string serviceUrl, throw new ArgumentNullException(nameof(credentials)); } - var connectorClient = CreateConnectorClient(serviceUrl, credentials); - var results = await connectorClient.Conversations.GetConversationsAsync(continuationToken, cancellationToken).ConfigureAwait(false); - return results; + using (var connectorClient = CreateConnectorClient(serviceUrl, credentials)) + { + var results = await connectorClient.Conversations.GetConversationsAsync(continuationToken, cancellationToken).ConfigureAwait(false); + return results; + } } /// @@ -1602,14 +1604,14 @@ private IConnectorClient CreateConnectorClient(string serviceUrl, AppCredentials ConnectorClient connectorClient; if (appCredentials != null) { - connectorClient = new ConnectorClient(new Uri(serviceUrl), appCredentials, customHttpClient: _httpClient); + connectorClient = new ConnectorClient(new Uri(serviceUrl), appCredentials, customHttpClient: _httpClient, disposeHttpClient: _httpClient == null); } else { var emptyCredentials = (ChannelProvider != null && ChannelProvider.IsGovernment()) ? MicrosoftGovernmentAppCredentials.Empty : MicrosoftAppCredentials.Empty; - connectorClient = new ConnectorClient(new Uri(serviceUrl), emptyCredentials, customHttpClient: _httpClient); + connectorClient = new ConnectorClient(new Uri(serviceUrl), emptyCredentials, customHttpClient: _httpClient, disposeHttpClient: _httpClient == null); } if (_connectorClientRetryPolicy != null) diff --git a/libraries/Microsoft.Bot.Builder/CloudAdapterBase.cs b/libraries/Microsoft.Bot.Builder/CloudAdapterBase.cs index 230e30e7c1..db31c0bb42 100644 --- a/libraries/Microsoft.Bot.Builder/CloudAdapterBase.cs +++ b/libraries/Microsoft.Bot.Builder/CloudAdapterBase.cs @@ -243,13 +243,17 @@ protected async Task ProcessProactiveAsync(ClaimsIdentity claimsIdentity, Conver var credentials = await _botFrameworkAuthentication.GetProactiveCredentialsAsync(claimsIdentity, audience, cancellationToken).ConfigureAwait(false); // Create the connector client to use for outbound requests. - var connectorClient = new ConnectorClient(new Uri(reference.ServiceUrl), credentials, _httpClient); - - // Create a turn context and run the pipeline. - using (var context = CreateTurnContext(reference.GetContinuationActivity(), claimsIdentity, audience, connectorClient, callback)) + using (var connectorClient = new ConnectorClient(new Uri(reference.ServiceUrl), credentials, _httpClient, disposeHttpClient: _httpClient == null)) { - // Run the pipeline. - await RunPipelineAsync(context, callback, cancellationToken).ConfigureAwait(false); + // Create a turn context and run the pipeline. + using (var context = CreateTurnContext(reference.GetContinuationActivity(), claimsIdentity, audience, connectorClient, callback)) + { + // Run the pipeline. + await RunPipelineAsync(context, callback, cancellationToken).ConfigureAwait(false); + + // Cleanup disposable resources in case other code kept a reference to it. + context.TurnState.Set(null); + } } } @@ -270,16 +274,20 @@ protected async Task ProcessActivityAsync(string authHeader, Act activity.CallerId = authenticateRequestResult.CallerId; // Create the connector client to use for outbound requests. - var connectorClient = new ConnectorClient(new Uri(activity.ServiceUrl), authenticateRequestResult.Credentials, _httpClient); - - // Create a turn context and run the pipeline. - using (var context = CreateTurnContext(activity, authenticateRequestResult.ClaimsIdentity, authenticateRequestResult.Scope, connectorClient, callback)) + using (var connectorClient = new ConnectorClient(new Uri(activity.ServiceUrl), authenticateRequestResult.Credentials, _httpClient, disposeHttpClient: _httpClient == null)) { - // Run the pipeline. - await RunPipelineAsync(context, callback, cancellationToken).ConfigureAwait(false); + // Create a turn context and run the pipeline. + using (var context = CreateTurnContext(activity, authenticateRequestResult.ClaimsIdentity, authenticateRequestResult.Scope, connectorClient, callback)) + { + // Run the pipeline. + await RunPipelineAsync(context, callback, cancellationToken).ConfigureAwait(false); - // If there are any results they will have been left on the TurnContext. - return ProcessTurnResults(context); + // Cleanup disposable resources in case other code kept a reference to it. + context.TurnState.Set(null); + + // If there are any results they will have been left on the TurnContext. + return ProcessTurnResults(context); + } } } diff --git a/libraries/Microsoft.Bot.Builder/Inspection/InspectionSession.cs b/libraries/Microsoft.Bot.Builder/Inspection/InspectionSession.cs index 9bb0697c5a..87eeb75f80 100644 --- a/libraries/Microsoft.Bot.Builder/Inspection/InspectionSession.cs +++ b/libraries/Microsoft.Bot.Builder/Inspection/InspectionSession.cs @@ -22,7 +22,7 @@ public InspectionSession(ConversationReference conversationReference, MicrosoftA { _conversationReference = conversationReference; _logger = logger; - _connectorClient = new ConnectorClient(new Uri(_conversationReference.ServiceUrl), credentials, httpClient); + _connectorClient = new ConnectorClient(new Uri(_conversationReference.ServiceUrl), credentials, httpClient, disposeHttpClient: httpClient == null); } public async Task SendAsync(Activity activity, CancellationToken cancellationToken) diff --git a/libraries/Microsoft.Bot.Builder/Streaming/BotFrameworkHttpAdapterBase.cs b/libraries/Microsoft.Bot.Builder/Streaming/BotFrameworkHttpAdapterBase.cs index 919c1c426f..bf69ee2e75 100644 --- a/libraries/Microsoft.Bot.Builder/Streaming/BotFrameworkHttpAdapterBase.cs +++ b/libraries/Microsoft.Bot.Builder/Streaming/BotFrameworkHttpAdapterBase.cs @@ -143,11 +143,17 @@ public async Task ProcessStreamingActivityAsync(Activity activit { context.TurnState.Add(BotIdentityKey, ClaimsIdentity); } - - var connectorClient = CreateStreamingConnectorClient(activity, requestHandler); - context.TurnState.Add(connectorClient); - await RunPipelineAsync(context, callbackHandler, cancellationToken).ConfigureAwait(false); + using (var connectorClient = CreateStreamingConnectorClient(activity, requestHandler)) + { + // Add connector client to be used throughout the turn + context.TurnState.Add(connectorClient); + + await RunPipelineAsync(context, callbackHandler, cancellationToken).ConfigureAwait(false); + + // Cleanup connector client + context.TurnState.Set(null); + } if (activity.Type == ActivityTypes.Invoke) { @@ -308,7 +314,7 @@ private IConnectorClient CreateStreamingConnectorClient(Activity activity, Strea #pragma warning disable CA2000 // Dispose objects before losing scope (We need to make ConnectorClient disposable to fix this, ignoring it for now) var streamingClient = new StreamingHttpClient(requestHandler, Logger); #pragma warning restore CA2000 // Dispose objects before losing scope - var connectorClient = new ConnectorClient(new Uri(activity.ServiceUrl), emptyCredentials, customHttpClient: streamingClient); + var connectorClient = new ConnectorClient(new Uri(activity.ServiceUrl), emptyCredentials, customHttpClient: streamingClient, disposeHttpClient: false); return connectorClient; } diff --git a/libraries/Microsoft.Bot.Builder/Streaming/StreamingRequestHandler.cs b/libraries/Microsoft.Bot.Builder/Streaming/StreamingRequestHandler.cs index cdecfaad59..c9046fde4e 100644 --- a/libraries/Microsoft.Bot.Builder/Streaming/StreamingRequestHandler.cs +++ b/libraries/Microsoft.Bot.Builder/Streaming/StreamingRequestHandler.cs @@ -225,7 +225,7 @@ public override async Task ProcessRequestAsync(ReceiveRequest string body; try { - body = request.ReadBodyAsString(); + body = await request.ReadBodyAsStringAsync().ConfigureAwait(false); } #pragma warning disable CA1031 // Do not catch general exception types (we log the exception and continue execution) catch (Exception ex) diff --git a/libraries/Microsoft.Bot.Connector/ConnectorClientEx.cs b/libraries/Microsoft.Bot.Connector/ConnectorClientEx.cs index e819ddaeee..7b05a12829 100644 --- a/libraries/Microsoft.Bot.Connector/ConnectorClientEx.cs +++ b/libraries/Microsoft.Bot.Connector/ConnectorClientEx.cs @@ -62,6 +62,34 @@ public ConnectorClient(Uri baseUri, MicrosoftAppCredentials credentials, HttpCli { } + /// + /// Initializes a new instance of the class. + /// + /// Base URI for the Bot Connector service. + /// Credentials for the Bot Connector service. + /// The HTTP client to use for this connector client. + /// Whether to dispose the . + /// Constructor specifically designed to be the one that allows control of the disposing of the custom . + /// only has one constructor that accepts control of the disposing of the , so we call that overload here. + /// All other overloads of will not control this parameter and it will default to true, resulting on disposal of the provided when the is disposed. + /// When reusing instances across connectors, pass 'false' for to avoid . +#pragma warning disable CA1801 // Review unused parameters (we can't change this without breaking binary compat) + public ConnectorClient(Uri baseUri, ServiceClientCredentials credentials, HttpClient customHttpClient, bool disposeHttpClient) +#pragma warning restore CA1801 // Review unused parameters + : base(customHttpClient, disposeHttpClient) + { + this.Credentials = credentials; + + if (baseUri == null) + { + throw new ArgumentNullException(nameof(baseUri)); + } + + Initialize(); + + BaseUri = baseUri; + } + /// /// Initializes a new instance of the class. /// diff --git a/libraries/Microsoft.Bot.Streaming/ReceiveRequestExtensions.cs b/libraries/Microsoft.Bot.Streaming/ReceiveRequestExtensions.cs index 86e5387fd0..fc290f715d 100644 --- a/libraries/Microsoft.Bot.Streaming/ReceiveRequestExtensions.cs +++ b/libraries/Microsoft.Bot.Streaming/ReceiveRequestExtensions.cs @@ -5,6 +5,7 @@ using System.IO; using System.Linq; using System.Text; +using System.Threading.Tasks; using Newtonsoft.Json; namespace Microsoft.Bot.Streaming @@ -23,6 +24,19 @@ public static class ReceiveRequestExtensions /// Otherwise a default instance of type T. /// public static T ReadBodyAsJson(this ReceiveRequest request) + { + return request.ReadBodyAsJsonAsync().GetAwaiter().GetResult(); + } + + /// + /// Serializes the body of this as JSON. + /// + /// The type to attempt to deserialize the contents of this 's body into. + /// The current instance of . + /// On success, an object of type T populated with data serialized from the body. + /// Otherwise a default instance of type T. + /// + public static async Task ReadBodyAsJsonAsync(this ReceiveRequest request) { // The first stream attached to a ReceiveRequest is always the ReceiveRequest body. // Any additional streams must be defined within the body or they will not be @@ -30,22 +44,17 @@ public static T ReadBodyAsJson(this ReceiveRequest request) var contentStream = request.Streams.FirstOrDefault(); /* If the response had no body we have to return a compatible - * but empty object to avoid throwing exceptions upstream anytime - * an empty response is received. - */ + * but empty object to avoid throwing exceptions upstream anytime + * an empty response is received. + */ if (contentStream == null) { return default; } - using (var reader = new StreamReader(contentStream.Stream, Encoding.UTF8)) - { - using (var jsonReader = new JsonTextReader(reader)) - { - var serializer = JsonSerializer.Create(SerializationSettings.DefaultDeserializationSettings); - return serializer.Deserialize(jsonReader); - } - } + var bodyString = await request.ReadBodyAsStringAsync().ConfigureAwait(false); + + return JsonConvert.DeserializeObject(bodyString, SerializationSettings.DefaultDeserializationSettings); } /// @@ -56,17 +65,29 @@ public static T ReadBodyAsJson(this ReceiveRequest request) /// Otherwise null. /// public static string ReadBodyAsString(this ReceiveRequest request) + { + return request.ReadBodyAsStringAsync().GetAwaiter().GetResult(); + } + + /// + /// Reads the body of this as a string. + /// + /// The current instance of . + /// On success, a string populated with data read from the body. + /// Otherwise null. + /// + public static Task ReadBodyAsStringAsync(this ReceiveRequest request) { var contentStream = request.Streams.FirstOrDefault(); if (contentStream == null) { - return string.Empty; + return Task.FromResult(string.Empty); } using (var reader = new StreamReader(contentStream.Stream, Encoding.UTF8)) { - return reader.ReadToEnd(); + return reader.ReadToEndAsync(); } } } diff --git a/libraries/Microsoft.Bot.Streaming/ReceiveResponseExtensions.cs b/libraries/Microsoft.Bot.Streaming/ReceiveResponseExtensions.cs index 3fddd5b783..2b8a521409 100644 --- a/libraries/Microsoft.Bot.Streaming/ReceiveResponseExtensions.cs +++ b/libraries/Microsoft.Bot.Streaming/ReceiveResponseExtensions.cs @@ -5,6 +5,7 @@ using System.IO; using System.Linq; using System.Text; +using System.Threading.Tasks; using Newtonsoft.Json; namespace Microsoft.Bot.Streaming @@ -52,12 +53,23 @@ public static T ReadBodyAsJson(this ReceiveResponse response) /// On success, an of the data from the body. /// public static string ReadBodyAsString(this ReceiveResponse response) + { + return response.ReadBodyAsStringAsync().GetAwaiter().GetResult(); + } + + /// + /// Serializes the body of this as a . + /// + /// The current instance of . + /// On success, an of the data from the body. + /// + public static async Task ReadBodyAsStringAsync(this ReceiveResponse response) { var contentStream = response.Streams.FirstOrDefault(); if (contentStream != null) { - return contentStream.Stream.ReadAsUtf8String(); + return await contentStream.Stream.ReadAsUtf8StringAsync().ConfigureAwait(false); } return string.Empty; diff --git a/tests/Microsoft.Bot.Connector.Tests/ConnectorClientTest.cs b/tests/Microsoft.Bot.Connector.Tests/ConnectorClientTest.cs index fb6ffd333e..c0adf6556e 100644 --- a/tests/Microsoft.Bot.Connector.Tests/ConnectorClientTest.cs +++ b/tests/Microsoft.Bot.Connector.Tests/ConnectorClientTest.cs @@ -3,6 +3,7 @@ using System; using System.Net.Http; +using System.Threading.Tasks; using Microsoft.Bot.Connector.Authentication; using Microsoft.Bot.Schema; using Xunit; @@ -12,7 +13,7 @@ namespace Microsoft.Bot.Connector.Tests public class ConnectorClientTest : BaseTest { [Fact] - public void ConnectorClientWithCustomHttpClientAndMicrosoftCredentials() + public void ConnectorClient_CustomHttpClient_AndMicrosoftCredentials() { var baseUri = new Uri("https://test.coffee"); var customHttpClient = new HttpClient(); @@ -23,5 +24,45 @@ public void ConnectorClientWithCustomHttpClientAndMicrosoftCredentials() Assert.Equal(connector.HttpClient.BaseAddress, baseUri); } + + [Fact] + public async Task ConnectorClient_CustomHttpClientAndCredConstructor_HttpClientDisposed() + { + var baseUri = new Uri("https://test.coffee"); + var customHttpClient = new HttpClient(); + + using (var connector = new ConnectorClient(new Uri("http://localhost/"), new MicrosoftAppCredentials(string.Empty, string.Empty), customHttpClient)) + { + // Use the connector + } + + await Assert.ThrowsAsync(() => customHttpClient.GetAsync("http://bing.com")); + } + + [Fact] + public async Task ConnectorClient_CustomHttpClientAndDisposeFalse_HttpClientNotDisposed() + { + var baseUri = new Uri("https://test.coffee"); + var customHttpClient = new HttpClient(); + + using (var connector = new ConnectorClient(new Uri("http://localhost/"), new MicrosoftAppCredentials(string.Empty, string.Empty), customHttpClient, disposeHttpClient: customHttpClient == null)) + { + // Use the connector + } + + // If the HttpClient were disposed, this would throw ObjectDisposedException + await customHttpClient.GetAsync("http://bing.com"); + } + + [Fact] + public void ConnectorClient_CustomHttpClientNull_Works() + { + var baseUri = new Uri("https://test.coffee"); + + using (var connector = new ConnectorClient(new Uri("http://localhost/"), new MicrosoftAppCredentials(string.Empty, string.Empty), null, disposeHttpClient: true)) + { + // Use the connector + } + } } }