Merge pull request #16438 from jtattermusch/csharp_fix_subchannel_sharing

C#: fix subchannel sharing for secure channels

src/csharp/Grpc.Core.Tests/ChannelCredentialsTest.cs

@@ -17,13 +17,7 @@
 #endregion
 
 using System;
-using System.Diagnostics;
-using System.Runtime.InteropServices;
-using System.Threading;
-using System.Threading.Tasks;
-using Grpc.Core;
 using Grpc.Core.Internal;
-using Grpc.Core.Utils;
 using NUnit.Framework;
 
 namespace Grpc.Core.Tests
@@ -44,9 +38,38 @@ namespace Grpc.Core.Tests
 
             Assert.Throws(typeof(ArgumentNullException), () => ChannelCredentials.Create(null, new FakeCallCredentials()));
             Assert.Throws(typeof(ArgumentNullException), () => ChannelCredentials.Create(new FakeChannelCredentials(true), null));
-            
+
             // forbid composing non-composable
             Assert.Throws(typeof(ArgumentException), () => ChannelCredentials.Create(new FakeChannelCredentials(false), new FakeCallCredentials()));
         }
+
+        [Test]
+        public void ChannelCredentials_NativeCredentialsAreReused()
+        {
+            // always returning the same native object is critical for subchannel sharing to work with secure channels
+            var creds = new SslCredentials();
+            var nativeCreds1 = creds.GetNativeCredentials();
+            var nativeCreds2 = creds.GetNativeCredentials();
+            Assert.AreSame(nativeCreds1, nativeCreds2);
+        }
+
+        [Test]
+        public void ChannelCredentials_CreateExceptionIsCached()
+        {
+            var creds = new ChannelCredentialsWithCreateNativeThrows();
+            var ex1 = Assert.Throws(typeof(Exception), () => creds.GetNativeCredentials());
+            var ex2 = Assert.Throws(typeof(Exception), () => creds.GetNativeCredentials());
+            Assert.AreSame(ex1, ex2);
+        }
+
+        internal class ChannelCredentialsWithCreateNativeThrows : ChannelCredentials
+        {
+            internal override bool IsComposable => false;
+
+            internal override ChannelCredentialsSafeHandle CreateNativeCredentials()
+            {
+                throw new Exception("Creation of native credentials has failed on purpose.");
+            }
+        }
     }
 }

src/csharp/Grpc.Core.Tests/FakeCredentials.cs

@@ -16,15 +16,7 @@
 
 #endregion
 
-using System;
-using System.Diagnostics;
-using System.Runtime.InteropServices;
-using System.Threading;
-using System.Threading.Tasks;
-using Grpc.Core;
 using Grpc.Core.Internal;
-using Grpc.Core.Utils;
-using NUnit.Framework;
 
 namespace Grpc.Core.Tests
 {
@@ -42,7 +34,7 @@ namespace Grpc.Core.Tests
             get { return composable; }
         }
 
-        internal override ChannelCredentialsSafeHandle ToNativeCredentials()
+        internal override ChannelCredentialsSafeHandle CreateNativeCredentials()
         {
             return null;
         }

src/csharp/Grpc.Core/Channel.cs

@@ -72,9 +72,9 @@ namespace Grpc.Core
             this.environment = GrpcEnvironment.AddRef();
 
             this.completionQueue = this.environment.PickCompletionQueue();
-            using (var nativeCredentials = credentials.ToNativeCredentials())
             using (var nativeChannelArgs = ChannelOptions.CreateChannelArgs(this.options.Values))
             {
+                var nativeCredentials = credentials.GetNativeCredentials();
                 if (nativeCredentials != null)
                 {
                     this.handle = ChannelSafeHandle.CreateSecure(nativeCredentials, target, nativeChannelArgs);

src/csharp/Grpc.Core/ChannelCredentials.cs

@@ -31,6 +31,19 @@ namespace Grpc.Core
     public abstract class ChannelCredentials
     {
         static readonly ChannelCredentials InsecureInstance = new InsecureCredentialsImpl();
+        readonly Lazy<ChannelCredentialsSafeHandle> cachedNativeCredentials;
+
+        /// <summary>
+        /// Creates a new instance of channel credentials
+        /// </summary>
+        public ChannelCredentials()
+        {
+            // Native credentials object need to be kept alive once initialized for subchannel sharing to work correctly
+            // with secure connections. See https://github.com/grpc/grpc/issues/15207.
+            // We rely on finalizer to clean up the native portion of ChannelCredentialsSafeHandle after the ChannelCredentials
+            // instance becomes unused.
+            this.cachedNativeCredentials = new Lazy<ChannelCredentialsSafeHandle>(() => CreateNativeCredentials());
+        }
 
         /// <summary>
         /// Returns instance of credentials that provides no security and 
@@ -57,11 +70,22 @@ namespace Grpc.Core
         }
 
         /// <summary>
-        /// Creates native object for the credentials. May return null if insecure channel
-        /// should be created.
+        /// Gets native object for the credentials, creating one if it already doesn't exist. May return null if insecure channel
+        /// should be created. Caller must not call <c>Dispose()</c> on the returned native credentials as their lifetime
+        /// is managed by this class (and instances of native credentials are cached).
+        /// </summary>
+        /// <returns>The native credentials.</returns>
+        internal ChannelCredentialsSafeHandle GetNativeCredentials()
+        {
+            return cachedNativeCredentials.Value;
+        }
+
+        /// <summary>
+        /// Creates a new native object for the credentials. May return null if insecure channel
+        /// should be created. For internal use only, use <see cref="GetNativeCredentials"/> instead.
         /// </summary>
         /// <returns>The native credentials.</returns>
-        internal abstract ChannelCredentialsSafeHandle ToNativeCredentials();
+        internal abstract ChannelCredentialsSafeHandle CreateNativeCredentials();
 
         /// <summary>
         /// Returns <c>true</c> if this credential type allows being composed by <c>CompositeCredentials</c>.
@@ -73,7 +97,7 @@ namespace Grpc.Core
 
         private sealed class InsecureCredentialsImpl : ChannelCredentials
         {
-            internal override ChannelCredentialsSafeHandle ToNativeCredentials()
+            internal override ChannelCredentialsSafeHandle CreateNativeCredentials()
             {
                 return null;
             }
@@ -145,7 +169,7 @@ namespace Grpc.Core
             get { return true; }
         }
 
-        internal override ChannelCredentialsSafeHandle ToNativeCredentials()
+        internal override ChannelCredentialsSafeHandle CreateNativeCredentials()
         {
             return ChannelCredentialsSafeHandle.CreateSslCredentials(rootCertificates, keyCertificatePair);
         }
@@ -173,12 +197,11 @@ namespace Grpc.Core
             GrpcPreconditions.CheckArgument(channelCredentials.IsComposable, "Supplied channel credentials do not allow composition.");
         }
 
-        internal override ChannelCredentialsSafeHandle ToNativeCredentials()
+        internal override ChannelCredentialsSafeHandle CreateNativeCredentials()
         {
-            using (var channelCreds = channelCredentials.ToNativeCredentials())
             using (var callCreds = callCredentials.ToNativeCredentials())
             {
-                var nativeComposite = ChannelCredentialsSafeHandle.CreateComposite(channelCreds, callCreds);
+                var nativeComposite = ChannelCredentialsSafeHandle.CreateComposite(channelCredentials.GetNativeCredentials(), callCreds);
                 if (nativeComposite.IsInvalid)
                 {
                     throw new ArgumentException("Error creating native composite credentials. Likely, this is because you are trying to compose incompatible credentials.");