Add channel setting for disposing HttpClient (#458)

JamesNK · web-flow · commit fae09a26c2f5 · 2019-08-21T07:16:13.000+12:00
diff --git a/global.json b/global.json
@@ -1,5 +1,5 @@
 {
   "sdk": {
-    "version": "3.0.100-preview9-013906"
+    "version": "3.0.100-preview9-013927"
   }
 }
diff --git a/src/Grpc.Net.Client/GrpcChannel.cs b/src/Grpc.Net.Client/GrpcChannel.cs
@@ -36,7 +36,7 @@ namespace Grpc.Net.Client
     /// Client objects can reuse the same channel. Creating a channel is an expensive operation compared to invoking
     /// a remote call so in general you should reuse a single channel for as many calls as possible.
     /// </summary>
-    public sealed class GrpcChannel : ChannelBase
+    public sealed class GrpcChannel : ChannelBase, IDisposable
     {
         internal const int DefaultMaxReceiveMessageSize = 1024 * 1024 * 4; // 4 MB
 
@@ -49,13 +49,21 @@ public sealed class GrpcChannel : ChannelBase
         internal List<CallCredentials>? CallCredentials { get; }
         internal Dictionary<string, ICompressionProvider> CompressionProviders { get; }
         internal string MessageAcceptEncoding { get; }
+        internal bool Disposed { get; private set; }
 
         // Timing related options that are set in unit tests
         internal ISystemClock Clock = SystemClock.Instance;
         internal bool DisableClientDeadlineTimer { get; set; }
 
+        private bool _shouldDisposeHttpClient;
+
         internal GrpcChannel(Uri address, GrpcChannelOptions channelOptions) : base(address.Authority)
         {
+            // Dispose the HttpClient if...
+            //   1. No client was specified and so the channel created the HttpClient itself
+            //   2. User has specified a client and set DisposeHttpClient to true
+            _shouldDisposeHttpClient = channelOptions.HttpClient == null || channelOptions.DisposeHttpClient;
+
             Address = address;
             HttpClient = channelOptions.HttpClient ?? new HttpClient();
             SendMaxMessageSize = channelOptions.MaxSendMessageSize;
@@ -117,6 +125,11 @@ private void ValidateChannelCredentials()
         /// <returns>A new <see cref="CallInvoker"/>.</returns>
         public override CallInvoker CreateCallInvoker()
         {
+            if (Disposed)
+            {
+                throw new ObjectDisposedException(nameof(GrpcChannel));
+            }
+
             var invoker = new HttpClientCallInvoker(this);
 
             return invoker;
@@ -211,5 +224,23 @@ public static GrpcChannel ForAddress(Uri address, GrpcChannelOptions channelOpti
 
             return new GrpcChannel(address, channelOptions);
         }
+
+        /// <summary>
+        /// Releases the resources used by the <see cref="GrpcChannel"/> class.
+        /// Clients created with the channel can't be used after the channel is disposed.
+        /// </summary>
+        public void Dispose()
+        {
+            if (Disposed)
+            {
+                return;
+            }
+
+            if (_shouldDisposeHttpClient)
+            {
+                HttpClient.Dispose();
+            }
+            Disposed = true;
+        }
     }
 }
diff --git a/src/Grpc.Net.Client/GrpcChannelOptions.cs b/src/Grpc.Net.Client/GrpcChannelOptions.cs
@@ -16,7 +16,6 @@
 
 #endregion
 
-using System.Collections;
 using System.Collections.Generic;
 using System.Net.Http;
 using Grpc.Core;
@@ -58,10 +57,26 @@ public sealed class GrpcChannelOptions
         /// <summary>
         /// Gets or sets the <see cref="HttpClient"/> used by the channel.
         /// </summary>
+        /// <remarks>
+        /// By default a <see cref="System.Net.Http.HttpClient"/> specified here will not be disposed with the channel.
+        /// To dispose the <see cref="System.Net.Http.HttpClient"/> with the channel you must set <see cref="DisposeHttpClient"/>
+        /// to <c>true</c>.
+        /// </remarks>
         public HttpClient? HttpClient { get; set; }
 
         /// <summary>
-        /// 
+        /// Gets or sets a value indicating whether the underlying <see cref="System.Net.Http.HttpClient"/> should be disposed
+        /// when the <see cref="GrpcChannel"/> instance is disposed. The default value is <c>false</c>.
+        /// </summary>
+        /// <remarks>
+        /// This setting is used when a <see cref="HttpClient"/> value is specified. If no <see cref="HttpClient"/> value is provided
+        /// then the channel will create an <see cref="System.Net.Http.HttpClient"/> instance that is always disposed when
+        /// the channel is disposed.
+        /// </remarks>
+        public bool DisposeHttpClient { get; set; }
+
+        /// <summary>
+        /// Initializes a new instance of the <see cref="GrpcChannelOptions"/> class.
         /// </summary>
         public GrpcChannelOptions()
         {
diff --git a/src/Grpc.Net.Client/Internal/HttpClientCallInvoker.cs b/src/Grpc.Net.Client/Internal/HttpClientCallInvoker.cs
@@ -119,6 +119,11 @@ private GrpcCall<TRequest, TResponse> CreateGrpcCall<TRequest, TResponse>(
             where TRequest : class
             where TResponse : class
         {
+            if (Channel.Disposed)
+            {
+                throw new ObjectDisposedException(nameof(GrpcChannel));
+            }
+
             var call = new GrpcCall<TRequest, TResponse>(method, options, Channel);
 
             return call;
diff --git a/test/FunctionalTests/Client/EventSourceTests.cs b/test/FunctionalTests/Client/EventSourceTests.cs
@@ -64,7 +64,7 @@ async Task<HelloReply> UnarySuccess(HelloRequest request, ServerCallContext cont
             // Act - Start call
             var method = Fixture.DynamicGrpc.AddUnaryMethod<HelloRequest, HelloReply>(UnarySuccess);
 
-            var client = TestClientFactory.Create(Fixture.Client, LoggerFactory, method);
+            var client = TestClientFactory.Create(Channel, method);
 
             var call = client.UnaryCall(new HelloRequest());
 
@@ -131,7 +131,7 @@ async Task<HelloReply> UnaryError(HelloRequest request, ServerCallContext contex
             // Act - Start call
             var method = Fixture.DynamicGrpc.AddUnaryMethod<HelloRequest, HelloReply>(UnaryError);
 
-            var client = TestClientFactory.Create(Fixture.Client, LoggerFactory, method);
+            var client = TestClientFactory.Create(Channel, method);
 
             var call = client.UnaryCall(new HelloRequest());
 
@@ -192,7 +192,10 @@ async Task<HelloReply> UnaryDeadlineExceeded(HelloRequest request, ServerCallCon
             // Act - Start call
             var method = Fixture.DynamicGrpc.AddUnaryMethod<HelloRequest, HelloReply>(UnaryDeadlineExceeded);
 
-            var client = TestClientFactory.Create(Fixture.Client, LoggerFactory, method, disableClientDeadlineTimer: true);
+            var channel = CreateChannel();
+            channel.DisableClientDeadlineTimer = true;
+
+            var client = TestClientFactory.Create(Channel, method);
 
             var call = client.UnaryCall(new HelloRequest(), new CallOptions(deadline: DateTime.UtcNow.AddMilliseconds(200)));
 
@@ -249,7 +252,7 @@ async Task DuplexStreamingMethod(IAsyncStreamReader<HelloRequest> requestStream,
             // Act - Start call
             var method = Fixture.DynamicGrpc.AddDuplexStreamingMethod<HelloRequest, HelloReply>(DuplexStreamingMethod);
 
-            var client = TestClientFactory.Create(Fixture.Client, LoggerFactory, method);
+            var client = TestClientFactory.Create(Channel, method);
 
             var call = client.DuplexStreamingCall();
 
diff --git a/test/FunctionalTests/Client/MaxMessageSizeTests.cs b/test/FunctionalTests/Client/MaxMessageSizeTests.cs
@@ -20,6 +20,7 @@
 using Greet;
 using Grpc.AspNetCore.FunctionalTests.Infrastructure;
 using Grpc.Core;
+using Grpc.Net.Client;
 using Grpc.Tests.Shared;
 using NUnit.Framework;
 
@@ -52,7 +53,10 @@ Task<HelloReply> UnaryDeadlineExceeded(HelloRequest request, ServerCallContext c
 
             var method = Fixture.DynamicGrpc.AddUnaryMethod<HelloRequest, HelloReply>(UnaryDeadlineExceeded);
 
-            var client = TestClientFactory.Create(Fixture.Client, LoggerFactory, method, disableClientDeadlineTimer: true);
+            var channel = CreateChannel();
+            channel.DisableClientDeadlineTimer = true;
+
+            var client = TestClientFactory.Create(channel, method);
 
             // Act
             var ex = await ExceptionAssert.ThrowsAsync<RpcException>(() => client.UnaryCall(new HelloRequest()).ResponseAsync).DefaultTimeout();
diff --git a/test/FunctionalTests/Client/StreamingTests.cs b/test/FunctionalTests/Client/StreamingTests.cs
@@ -23,7 +23,6 @@
 using System.Threading.Tasks;
 using Google.Protobuf;
 using Grpc.Core;
-using Grpc.Net.Client;
 using Grpc.Tests.Shared;
 using Microsoft.Extensions.Logging;
 using NUnit.Framework;
diff --git a/test/FunctionalTests/FunctionalTestBase.cs b/test/FunctionalTests/FunctionalTestBase.cs
@@ -38,21 +38,15 @@ public class FunctionalTestBase
 
         protected ILogger Logger => _testContext!.Logger;
 
-        protected GrpcChannel Channel
+        protected GrpcChannel Channel => _channel ??= CreateChannel();
+
+        protected GrpcChannel CreateChannel()
         {
-            get
+            return GrpcChannel.ForAddress(Fixture.Client.BaseAddress, new GrpcChannelOptions
             {
-                if (_channel == null)
-                {
-                    _channel = GrpcChannel.ForAddress(Fixture.Client.BaseAddress, new GrpcChannelOptions
-                    {
-                        LoggerFactory = LoggerFactory,
-                        HttpClient = Fixture.Client
-                    });
-                }
-
-                return _channel;
-            }
+                LoggerFactory = LoggerFactory,
+                HttpClient = Fixture.Client
+            });
         }
 
         protected virtual void ConfigureServices(IServiceCollection services) { }
diff --git a/test/FunctionalTests/Infrastructure/TestClient.cs b/test/FunctionalTests/Infrastructure/TestClient.cs
@@ -28,19 +28,12 @@ internal class TestClient<TRequest, TResponse> : ClientBase
         where TRequest : class
         where TResponse : class
     {
-        private readonly HttpClientCallInvoker _callInvoker;
+        private readonly CallInvoker _callInvoker;
         private readonly Method<TRequest, TResponse> _method;
 
-        public TestClient(HttpClient httpClient, ILoggerFactory loggerFactory, Method<TRequest, TResponse> method, bool disableClientDeadlineTimer = false)
+        public TestClient(ChannelBase channel, Method<TRequest, TResponse> method)
         {
-            var channel = GrpcChannel.ForAddress(httpClient.BaseAddress, new GrpcChannelOptions
-            {
-                LoggerFactory = loggerFactory,
-                HttpClient = httpClient
-            });
-            channel.DisableClientDeadlineTimer = disableClientDeadlineTimer;
-
-            _callInvoker = new HttpClientCallInvoker(channel);
+            _callInvoker = channel.CreateCallInvoker();
             _method = method;
         }
 
@@ -68,14 +61,12 @@ public AsyncDuplexStreamingCall<TRequest, TResponse> DuplexStreamingCall(CallOpt
     internal static class TestClientFactory
     {
         public static TestClient<TRequest, TResponse> Create<TRequest, TResponse>(
-            HttpClient httpClient,
-            ILoggerFactory loggerFactory,
-            Method<TRequest, TResponse> method,
-            bool disableClientDeadlineTimer = false)
+            ChannelBase channel,
+            Method<TRequest, TResponse> method)
             where TRequest : class
             where TResponse : class
         {
-            return new TestClient<TRequest, TResponse>(httpClient, loggerFactory, method, disableClientDeadlineTimer);
+            return new TestClient<TRequest, TResponse>(channel, method);
         }
     }
 }
diff --git a/test/Grpc.Net.Client.Tests/GrpcChannelTests.cs b/test/Grpc.Net.Client.Tests/GrpcChannelTests.cs

Original file line number	Diff line number	Diff line change
`@@ -1,5 +1,5 @@`
`1`	`1`	`{`
`2`	`2`	`"sdk": {`
`3`		`- "version": "3.0.100-preview9-013906"`
	`3`	`+ "version": "3.0.100-preview9-013927"`
`4`	`4`	`}`
`5`	`5`	`}`
Original file line number	Diff line number	Diff line change
`@@ -28,19 +28,12 @@ internal class TestClient<TRequest, TResponse> : ClientBase`
`28`	`28`	`where TRequest : class`
`29`	`29`	`where TResponse : class`
`30`	`30`	`{`
`31`		`- private readonly HttpClientCallInvoker _callInvoker;`
	`31`	`+ private readonly CallInvoker _callInvoker;`
`32`	`32`	`private readonly Method<TRequest, TResponse> _method;`
`33`	`33`
`34`		`- public TestClient(HttpClient httpClient, ILoggerFactory loggerFactory, Method<TRequest, TResponse> method, bool disableClientDeadlineTimer = false)`
	`34`	`+ public TestClient(ChannelBase channel, Method<TRequest, TResponse> method)`
`35`	`35`	`{`
`36`		`- var channel = GrpcChannel.ForAddress(httpClient.BaseAddress, new GrpcChannelOptions`
`37`		`- {`
`38`		`- LoggerFactory = loggerFactory,`
`39`		`- HttpClient = httpClient`
`40`		`- });`
`41`		`- channel.DisableClientDeadlineTimer = disableClientDeadlineTimer;`
`42`		`-`
`43`		`- _callInvoker = new HttpClientCallInvoker(channel);`
	`36`	`+ _callInvoker = channel.CreateCallInvoker();`
`44`	`37`	`_method = method;`
`45`	`38`	`}`
`46`	`39`
`@@ -68,14 +61,12 @@ public AsyncDuplexStreamingCall<TRequest, TResponse> DuplexStreamingCall(CallOpt`
`68`	`61`	`internal static class TestClientFactory`
`69`	`62`	`{`
`70`	`63`	`public static TestClient<TRequest, TResponse> Create<TRequest, TResponse>(`
`71`		`- HttpClient httpClient,`
`72`		`- ILoggerFactory loggerFactory,`
`73`		`- Method<TRequest, TResponse> method,`
`74`		`- bool disableClientDeadlineTimer = false)`
	`64`	`+ ChannelBase channel,`
	`65`	`+ Method<TRequest, TResponse> method)`
`75`	`66`	`where TRequest : class`
`76`	`67`	`where TResponse : class`
`77`	`68`	`{`
`78`		`- return new TestClient<TRequest, TResponse>(httpClient, loggerFactory, method, disableClientDeadlineTimer);`
	`69`	`+ return new TestClient<TRequest, TResponse>(channel, method);`
`79`	`70`	`}`
`80`	`71`	`}`
`81`	`72`	`}`