diff options
Diffstat (limited to 'src/csharp')
22 files changed, 422 insertions, 127 deletions
diff --git a/src/csharp/Grpc.Core.Tests/ChannelConnectivityTest.cs b/src/csharp/Grpc.Core.Tests/ChannelConnectivityTest.cs index a43040f01a..0834ddadda 100644 --- a/src/csharp/Grpc.Core.Tests/ChannelConnectivityTest.cs +++ b/src/csharp/Grpc.Core.Tests/ChannelConnectivityTest.cs @@ -57,19 +57,23 @@ namespace Grpc.Core.Tests [Test] public async Task Channel_WaitForStateChangedAsync() { - helper.UnaryHandler = new UnaryServerMethod<string, string>((request, context) => - { - return Task.FromResult(request); - }); - Assert.ThrowsAsync(typeof(TaskCanceledException), - async () => await channel.WaitForStateChangedAsync(channel.State, DateTime.UtcNow.AddMilliseconds(10))); + async () => await channel.WaitForStateChangedAsync(channel.State, DateTime.UtcNow.AddMilliseconds(0))); var stateChangedTask = channel.WaitForStateChangedAsync(channel.State); + await channel.ConnectAsync(DateTime.UtcNow.AddMilliseconds(5000)); + await stateChangedTask; + Assert.AreEqual(ChannelState.Ready, channel.State); + } - await Calls.AsyncUnaryCall(helper.CreateUnaryCall(), "abc"); + [Test] + public async Task Channel_TryWaitForStateChangedAsync() + { + Assert.IsFalse(await channel.TryWaitForStateChangedAsync(channel.State, DateTime.UtcNow.AddMilliseconds(0))); - await stateChangedTask; + var stateChangedTask = channel.TryWaitForStateChangedAsync(channel.State); + await channel.ConnectAsync(DateTime.UtcNow.AddMilliseconds(5000)); + Assert.IsTrue(await stateChangedTask); Assert.AreEqual(ChannelState.Ready, channel.State); } diff --git a/src/csharp/Grpc.Core.Tests/ChannelCredentialsTest.cs b/src/csharp/Grpc.Core.Tests/ChannelCredentialsTest.cs index 62cc904a61..843d88bfb6 100644 --- a/src/csharp/Grpc.Core.Tests/ChannelCredentialsTest.cs +++ b/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."); + } + } } } diff --git a/src/csharp/Grpc.Core.Tests/FakeCredentials.cs b/src/csharp/Grpc.Core.Tests/FakeCredentials.cs index 7d658576e5..f23c9e9757 100644 --- a/src/csharp/Grpc.Core.Tests/FakeCredentials.cs +++ b/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; } diff --git a/src/csharp/Grpc.Core.Tests/Internal/AsyncCallTest.cs b/src/csharp/Grpc.Core.Tests/Internal/AsyncCallTest.cs index 9aab54d2d0..775849d89b 100644 --- a/src/csharp/Grpc.Core.Tests/Internal/AsyncCallTest.cs +++ b/src/csharp/Grpc.Core.Tests/Internal/AsyncCallTest.cs @@ -107,6 +107,42 @@ namespace Grpc.Core.Internal.Tests } [Test] + public void AsyncUnary_RequestSerializationExceptionDoesntLeakResources() + { + string nullRequest = null; // will throw when serializing + Assert.Throws(typeof(ArgumentNullException), () => asyncCall.UnaryCallAsync(nullRequest)); + Assert.AreEqual(0, channel.GetCallReferenceCount()); + Assert.IsTrue(fakeCall.IsDisposed); + } + + [Test] + public void AsyncUnary_StartCallFailureDoesntLeakResources() + { + fakeCall.MakeStartCallFail(); + Assert.Throws(typeof(InvalidOperationException), () => asyncCall.UnaryCallAsync("request1")); + Assert.AreEqual(0, channel.GetCallReferenceCount()); + Assert.IsTrue(fakeCall.IsDisposed); + } + + [Test] + public void SyncUnary_RequestSerializationExceptionDoesntLeakResources() + { + string nullRequest = null; // will throw when serializing + Assert.Throws(typeof(ArgumentNullException), () => asyncCall.UnaryCall(nullRequest)); + Assert.AreEqual(0, channel.GetCallReferenceCount()); + Assert.IsTrue(fakeCall.IsDisposed); + } + + [Test] + public void SyncUnary_StartCallFailureDoesntLeakResources() + { + fakeCall.MakeStartCallFail(); + Assert.Throws(typeof(InvalidOperationException), () => asyncCall.UnaryCall("request1")); + Assert.AreEqual(0, channel.GetCallReferenceCount()); + Assert.IsTrue(fakeCall.IsDisposed); + } + + [Test] public void ClientStreaming_StreamingReadNotAllowed() { asyncCall.ClientStreamingCallAsync(); @@ -328,6 +364,15 @@ namespace Grpc.Core.Internal.Tests } [Test] + public void ClientStreaming_StartCallFailureDoesntLeakResources() + { + fakeCall.MakeStartCallFail(); + Assert.Throws(typeof(InvalidOperationException), () => asyncCall.ClientStreamingCallAsync()); + Assert.AreEqual(0, channel.GetCallReferenceCount()); + Assert.IsTrue(fakeCall.IsDisposed); + } + + [Test] public void ServerStreaming_StreamingSendNotAllowed() { asyncCall.StartServerStreamingCall("request1"); @@ -402,6 +447,27 @@ namespace Grpc.Core.Internal.Tests } [Test] + public void ServerStreaming_RequestSerializationExceptionDoesntLeakResources() + { + string nullRequest = null; // will throw when serializing + Assert.Throws(typeof(ArgumentNullException), () => asyncCall.StartServerStreamingCall(nullRequest)); + Assert.AreEqual(0, channel.GetCallReferenceCount()); + Assert.IsTrue(fakeCall.IsDisposed); + + var responseStream = new ClientResponseStream<string, string>(asyncCall); + var readTask = responseStream.MoveNext(); + } + + [Test] + public void ServerStreaming_StartCallFailureDoesntLeakResources() + { + fakeCall.MakeStartCallFail(); + Assert.Throws(typeof(InvalidOperationException), () => asyncCall.StartServerStreamingCall("request1")); + Assert.AreEqual(0, channel.GetCallReferenceCount()); + Assert.IsTrue(fakeCall.IsDisposed); + } + + [Test] public void DuplexStreaming_NoRequestNoResponse_Success() { asyncCall.StartDuplexStreamingCall(); @@ -558,6 +624,15 @@ namespace Grpc.Core.Internal.Tests AssertStreamingResponseError(asyncCall, fakeCall, readTask2, StatusCode.Cancelled); } + [Test] + public void DuplexStreaming_StartCallFailureDoesntLeakResources() + { + fakeCall.MakeStartCallFail(); + Assert.Throws(typeof(InvalidOperationException), () => asyncCall.StartDuplexStreamingCall()); + Assert.AreEqual(0, channel.GetCallReferenceCount()); + Assert.IsTrue(fakeCall.IsDisposed); + } + ClientSideStatus CreateClientSideStatus(StatusCode statusCode) { return new ClientSideStatus(new Status(statusCode, ""), new Metadata()); diff --git a/src/csharp/Grpc.Core.Tests/Internal/FakeNativeCall.cs b/src/csharp/Grpc.Core.Tests/Internal/FakeNativeCall.cs index 581ac3384b..ef67918dab 100644 --- a/src/csharp/Grpc.Core.Tests/Internal/FakeNativeCall.cs +++ b/src/csharp/Grpc.Core.Tests/Internal/FakeNativeCall.cs @@ -31,6 +31,7 @@ namespace Grpc.Core.Internal.Tests /// </summary> internal class FakeNativeCall : INativeCall { + private bool shouldStartCallFail; public IUnaryResponseClientCallback UnaryResponseClientCallback { get; @@ -102,26 +103,31 @@ namespace Grpc.Core.Internal.Tests public void StartUnary(IUnaryResponseClientCallback callback, byte[] payload, WriteFlags writeFlags, MetadataArraySafeHandle metadataArray, CallFlags callFlags) { + StartCallMaybeFail(); UnaryResponseClientCallback = callback; } public void StartUnary(BatchContextSafeHandle ctx, byte[] payload, WriteFlags writeFlags, MetadataArraySafeHandle metadataArray, CallFlags callFlags) { + StartCallMaybeFail(); throw new NotImplementedException(); } public void StartClientStreaming(IUnaryResponseClientCallback callback, MetadataArraySafeHandle metadataArray, CallFlags callFlags) { + StartCallMaybeFail(); UnaryResponseClientCallback = callback; } public void StartServerStreaming(IReceivedStatusOnClientCallback callback, byte[] payload, WriteFlags writeFlags, MetadataArraySafeHandle metadataArray, CallFlags callFlags) { + StartCallMaybeFail(); ReceivedStatusOnClientCallback = callback; } public void StartDuplexStreaming(IReceivedStatusOnClientCallback callback, MetadataArraySafeHandle metadataArray, CallFlags callFlags) { + StartCallMaybeFail(); ReceivedStatusOnClientCallback = callback; } @@ -165,5 +171,22 @@ namespace Grpc.Core.Internal.Tests { IsDisposed = true; } + + /// <summary> + /// Emulate CallSafeHandle.CheckOk() failure for all future attempts + /// to start a call. + /// </summary> + public void MakeStartCallFail() + { + shouldStartCallFail = true; + } + + private void StartCallMaybeFail() + { + if (shouldStartCallFail) + { + throw new InvalidOperationException("Start call has failed."); + } + } } } diff --git a/src/csharp/Grpc.Core.Tests/MetadataTest.cs b/src/csharp/Grpc.Core.Tests/MetadataTest.cs index 8916731757..171c5c470e 100644 --- a/src/csharp/Grpc.Core.Tests/MetadataTest.cs +++ b/src/csharp/Grpc.Core.Tests/MetadataTest.cs @@ -66,6 +66,8 @@ namespace Grpc.Core.Tests new Metadata.Entry("0123456789abc", "XYZ"); new Metadata.Entry("-abc", "XYZ"); new Metadata.Entry("a_bc_", "XYZ"); + new Metadata.Entry("abc.xyz", "XYZ"); + new Metadata.Entry("abc.xyz-bin", new byte[] {1, 2, 3}); Assert.Throws(typeof(ArgumentException), () => new Metadata.Entry("abc[", "xyz")); Assert.Throws(typeof(ArgumentException), () => new Metadata.Entry("abc/", "xyz")); } diff --git a/src/csharp/Grpc.Core/Channel.cs b/src/csharp/Grpc.Core/Channel.cs index e9930b6fbc..7ce929dfa3 100644 --- a/src/csharp/Grpc.Core/Channel.cs +++ b/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); @@ -136,7 +136,7 @@ namespace Grpc.Core /// </summary> public async Task WaitForStateChangedAsync(ChannelState lastObservedState, DateTime? deadline = null) { - var result = await WaitForStateChangedInternalAsync(lastObservedState, deadline).ConfigureAwait(false); + var result = await TryWaitForStateChangedAsync(lastObservedState, deadline).ConfigureAwait(false); if (!result) { throw new TaskCanceledException("Reached deadline."); @@ -147,7 +147,7 @@ namespace Grpc.Core /// Returned tasks completes once channel state has become different from /// given lastObservedState (<c>true</c> is returned) or if the wait has timed out (<c>false</c> is returned). /// </summary> - internal Task<bool> WaitForStateChangedInternalAsync(ChannelState lastObservedState, DateTime? deadline = null) + public Task<bool> TryWaitForStateChangedAsync(ChannelState lastObservedState, DateTime? deadline = null) { GrpcPreconditions.CheckArgument(lastObservedState != ChannelState.Shutdown, "Shutdown is a terminal state. No further state changes can occur."); @@ -297,6 +297,12 @@ namespace Grpc.Core activeCallCounter.Decrement(); } + // for testing only + internal long GetCallReferenceCount() + { + return activeCallCounter.Count; + } + private ChannelState GetConnectivityState(bool tryToConnect) { try diff --git a/src/csharp/Grpc.Core/ChannelCredentials.cs b/src/csharp/Grpc.Core/ChannelCredentials.cs index ba482897d7..3ce32f31b7 100644 --- a/src/csharp/Grpc.Core/ChannelCredentials.cs +++ b/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."); diff --git a/src/csharp/Grpc.Core/Internal/AsyncCall.cs b/src/csharp/Grpc.Core/Internal/AsyncCall.cs index 9946d1a6cf..4cdf0ee6a7 100644 --- a/src/csharp/Grpc.Core/Internal/AsyncCall.cs +++ b/src/csharp/Grpc.Core/Internal/AsyncCall.cs @@ -17,6 +17,7 @@ #endregion using System; +using System.Threading; using System.Threading.Tasks; using Grpc.Core.Logging; using Grpc.Core.Profiling; @@ -34,6 +35,8 @@ namespace Grpc.Core.Internal readonly CallInvocationDetails<TRequest, TResponse> details; readonly INativeCall injectedNativeCall; // for testing + bool registeredWithChannel; + // Dispose of to de-register cancellation token registration IDisposable cancellationTokenRegistration; @@ -77,43 +80,59 @@ namespace Grpc.Core.Internal using (profiler.NewScope("AsyncCall.UnaryCall")) using (CompletionQueueSafeHandle cq = CompletionQueueSafeHandle.CreateSync()) { - byte[] payload = UnsafeSerialize(msg); + bool callStartedOk = false; + try + { + unaryResponseTcs = new TaskCompletionSource<TResponse>(); - unaryResponseTcs = new TaskCompletionSource<TResponse>(); + lock (myLock) + { + GrpcPreconditions.CheckState(!started); + started = true; + Initialize(cq); - lock (myLock) - { - GrpcPreconditions.CheckState(!started); - started = true; - Initialize(cq); + halfcloseRequested = true; + readingDone = true; + } - halfcloseRequested = true; - readingDone = true; - } + byte[] payload = UnsafeSerialize(msg); - using (var metadataArray = MetadataArraySafeHandle.Create(details.Options.Headers)) - { - var ctx = details.Channel.Environment.BatchContextPool.Lease(); - try + using (var metadataArray = MetadataArraySafeHandle.Create(details.Options.Headers)) { - call.StartUnary(ctx, payload, GetWriteFlagsForCall(), metadataArray, details.Options.Flags); - var ev = cq.Pluck(ctx.Handle); - bool success = (ev.success != 0); + var ctx = details.Channel.Environment.BatchContextPool.Lease(); try { - using (profiler.NewScope("AsyncCall.UnaryCall.HandleBatch")) + call.StartUnary(ctx, payload, GetWriteFlagsForCall(), metadataArray, details.Options.Flags); + callStartedOk = true; + + var ev = cq.Pluck(ctx.Handle); + bool success = (ev.success != 0); + try { - HandleUnaryResponse(success, ctx.GetReceivedStatusOnClient(), ctx.GetReceivedMessage(), ctx.GetReceivedInitialMetadata()); + using (profiler.NewScope("AsyncCall.UnaryCall.HandleBatch")) + { + HandleUnaryResponse(success, ctx.GetReceivedStatusOnClient(), ctx.GetReceivedMessage(), ctx.GetReceivedInitialMetadata()); + } + } + catch (Exception e) + { + Logger.Error(e, "Exception occurred while invoking completion delegate."); } } - catch (Exception e) + finally { - Logger.Error(e, "Exception occured while invoking completion delegate."); + ctx.Recycle(); } } - finally + } + finally + { + if (!callStartedOk) { - ctx.Recycle(); + lock (myLock) + { + OnFailedToStartCallLocked(); + } } } @@ -130,22 +149,35 @@ namespace Grpc.Core.Internal { lock (myLock) { - GrpcPreconditions.CheckState(!started); - started = true; + bool callStartedOk = false; + try + { + GrpcPreconditions.CheckState(!started); + started = true; - Initialize(details.Channel.CompletionQueue); + Initialize(details.Channel.CompletionQueue); - halfcloseRequested = true; - readingDone = true; + halfcloseRequested = true; + readingDone = true; - byte[] payload = UnsafeSerialize(msg); + byte[] payload = UnsafeSerialize(msg); - unaryResponseTcs = new TaskCompletionSource<TResponse>(); - using (var metadataArray = MetadataArraySafeHandle.Create(details.Options.Headers)) + unaryResponseTcs = new TaskCompletionSource<TResponse>(); + using (var metadataArray = MetadataArraySafeHandle.Create(details.Options.Headers)) + { + call.StartUnary(UnaryResponseClientCallback, payload, GetWriteFlagsForCall(), metadataArray, details.Options.Flags); + callStartedOk = true; + } + + return unaryResponseTcs.Task; + } + finally { - call.StartUnary(UnaryResponseClientCallback, payload, GetWriteFlagsForCall(), metadataArray, details.Options.Flags); + if (!callStartedOk) + { + OnFailedToStartCallLocked(); + } } - return unaryResponseTcs.Task; } } @@ -157,20 +189,32 @@ namespace Grpc.Core.Internal { lock (myLock) { - GrpcPreconditions.CheckState(!started); - started = true; + bool callStartedOk = false; + try + { + GrpcPreconditions.CheckState(!started); + started = true; - Initialize(details.Channel.CompletionQueue); + Initialize(details.Channel.CompletionQueue); - readingDone = true; + readingDone = true; + + unaryResponseTcs = new TaskCompletionSource<TResponse>(); + using (var metadataArray = MetadataArraySafeHandle.Create(details.Options.Headers)) + { + call.StartClientStreaming(UnaryResponseClientCallback, metadataArray, details.Options.Flags); + callStartedOk = true; + } - unaryResponseTcs = new TaskCompletionSource<TResponse>(); - using (var metadataArray = MetadataArraySafeHandle.Create(details.Options.Headers)) + return unaryResponseTcs.Task; + } + finally { - call.StartClientStreaming(UnaryResponseClientCallback, metadataArray, details.Options.Flags); + if (!callStartedOk) + { + OnFailedToStartCallLocked(); + } } - - return unaryResponseTcs.Task; } } @@ -181,21 +225,33 @@ namespace Grpc.Core.Internal { lock (myLock) { - GrpcPreconditions.CheckState(!started); - started = true; + bool callStartedOk = false; + try + { + GrpcPreconditions.CheckState(!started); + started = true; - Initialize(details.Channel.CompletionQueue); + Initialize(details.Channel.CompletionQueue); - halfcloseRequested = true; + halfcloseRequested = true; - byte[] payload = UnsafeSerialize(msg); + byte[] payload = UnsafeSerialize(msg); - streamingResponseCallFinishedTcs = new TaskCompletionSource<object>(); - using (var metadataArray = MetadataArraySafeHandle.Create(details.Options.Headers)) + streamingResponseCallFinishedTcs = new TaskCompletionSource<object>(); + using (var metadataArray = MetadataArraySafeHandle.Create(details.Options.Headers)) + { + call.StartServerStreaming(ReceivedStatusOnClientCallback, payload, GetWriteFlagsForCall(), metadataArray, details.Options.Flags); + callStartedOk = true; + } + call.StartReceiveInitialMetadata(ReceivedResponseHeadersCallback); + } + finally { - call.StartServerStreaming(ReceivedStatusOnClientCallback, payload, GetWriteFlagsForCall(), metadataArray, details.Options.Flags); + if (!callStartedOk) + { + OnFailedToStartCallLocked(); + } } - call.StartReceiveInitialMetadata(ReceivedResponseHeadersCallback); } } @@ -207,17 +263,29 @@ namespace Grpc.Core.Internal { lock (myLock) { - GrpcPreconditions.CheckState(!started); - started = true; + bool callStartedOk = false; + try + { + GrpcPreconditions.CheckState(!started); + started = true; - Initialize(details.Channel.CompletionQueue); + Initialize(details.Channel.CompletionQueue); - streamingResponseCallFinishedTcs = new TaskCompletionSource<object>(); - using (var metadataArray = MetadataArraySafeHandle.Create(details.Options.Headers)) + streamingResponseCallFinishedTcs = new TaskCompletionSource<object>(); + using (var metadataArray = MetadataArraySafeHandle.Create(details.Options.Headers)) + { + call.StartDuplexStreaming(ReceivedStatusOnClientCallback, metadataArray, details.Options.Flags); + callStartedOk = true; + } + call.StartReceiveInitialMetadata(ReceivedResponseHeadersCallback); + } + finally { - call.StartDuplexStreaming(ReceivedStatusOnClientCallback, metadataArray, details.Options.Flags); + if (!callStartedOk) + { + OnFailedToStartCallLocked(); + } } - call.StartReceiveInitialMetadata(ReceivedResponseHeadersCallback); } } @@ -325,9 +393,22 @@ namespace Grpc.Core.Internal } } - protected override void OnAfterReleaseResources() + protected override void OnAfterReleaseResourcesLocked() { - details.Channel.RemoveCallReference(this); + if (registeredWithChannel) + { + details.Channel.RemoveCallReference(this); + registeredWithChannel = false; + } + } + + protected override void OnAfterReleaseResourcesUnlocked() + { + // If cancellation callback is in progress, this can block + // so we need to do this outside of call's lock to prevent + // deadlock. + // See https://github.com/grpc/grpc/issues/14777 + // See https://github.com/dotnet/corefx/issues/14903 cancellationTokenRegistration?.Dispose(); } @@ -385,10 +466,27 @@ namespace Grpc.Core.Internal var call = CreateNativeCall(cq); details.Channel.AddCallReference(this); + registeredWithChannel = true; InitializeInternal(call); + RegisterCancellationCallback(); } + private void OnFailedToStartCallLocked() + { + ReleaseResources(); + + // We need to execute the hook that disposes the cancellation token + // registration, but it cannot be done from under a lock. + // To make things simple, we just schedule the unregistering + // on a threadpool. + // - Once the native call is disposed, the Cancel() calls are ignored anyway + // - We don't care about the overhead as OnFailedToStartCallLocked() only happens + // when something goes very bad when initializing a call and that should + // never happen when gRPC is used correctly. + ThreadPool.QueueUserWorkItem((state) => OnAfterReleaseResourcesUnlocked()); + } + private INativeCall CreateNativeCall(CompletionQueueSafeHandle cq) { if (injectedNativeCall != null) @@ -448,6 +546,7 @@ namespace Grpc.Core.Internal TResponse msg = default(TResponse); var deserializeException = TryDeserialize(receivedMessage, out msg); + bool releasedResources; lock (myLock) { finished = true; @@ -464,7 +563,12 @@ namespace Grpc.Core.Internal streamingWriteTcs = null; } - ReleaseResourcesIfPossible(); + releasedResources = ReleaseResourcesIfPossible(); + } + + if (releasedResources) + { + OnAfterReleaseResourcesUnlocked(); } responseHeadersTcs.SetResult(responseHeaders); @@ -494,6 +598,7 @@ namespace Grpc.Core.Internal TaskCompletionSource<object> delayedStreamingWriteTcs = null; + bool releasedResources; lock (myLock) { finished = true; @@ -504,7 +609,12 @@ namespace Grpc.Core.Internal streamingWriteTcs = null; } - ReleaseResourcesIfPossible(); + releasedResources = ReleaseResourcesIfPossible(); + } + + if (releasedResources) + { + OnAfterReleaseResourcesUnlocked(); } if (delayedStreamingWriteTcs != null) diff --git a/src/csharp/Grpc.Core/Internal/AsyncCallBase.cs b/src/csharp/Grpc.Core/Internal/AsyncCallBase.cs index 3273c26b88..a93dc34620 100644 --- a/src/csharp/Grpc.Core/Internal/AsyncCallBase.cs +++ b/src/csharp/Grpc.Core/Internal/AsyncCallBase.cs @@ -189,17 +189,21 @@ namespace Grpc.Core.Internal /// </summary> protected abstract Exception GetRpcExceptionClientOnly(); - private void ReleaseResources() + protected void ReleaseResources() { if (call != null) { call.Dispose(); } disposed = true; - OnAfterReleaseResources(); + OnAfterReleaseResourcesLocked(); } - protected virtual void OnAfterReleaseResources() + protected virtual void OnAfterReleaseResourcesLocked() + { + } + + protected virtual void OnAfterReleaseResourcesUnlocked() { } @@ -235,6 +239,7 @@ namespace Grpc.Core.Internal { bool delayCompletion = false; TaskCompletionSource<object> origTcs = null; + bool releasedResources; lock (myLock) { if (!success && !finished && IsClient) { @@ -252,7 +257,12 @@ namespace Grpc.Core.Internal streamingWriteTcs = null; } - ReleaseResourcesIfPossible(); + releasedResources = ReleaseResourcesIfPossible(); + } + + if (releasedResources) + { + OnAfterReleaseResourcesUnlocked(); } if (!success) @@ -282,9 +292,15 @@ namespace Grpc.Core.Internal /// </summary> protected void HandleSendStatusFromServerFinished(bool success) { + bool releasedResources; lock (myLock) { - ReleaseResourcesIfPossible(); + releasedResources = ReleaseResourcesIfPossible(); + } + + if (releasedResources) + { + OnAfterReleaseResourcesUnlocked(); } if (!success) @@ -310,6 +326,7 @@ namespace Grpc.Core.Internal var deserializeException = (success && receivedMessage != null) ? TryDeserialize(receivedMessage, out msg) : null; TaskCompletionSource<TRead> origTcs = null; + bool releasedResources; lock (myLock) { origTcs = streamingReadTcs; @@ -332,7 +349,12 @@ namespace Grpc.Core.Internal streamingReadTcs = null; } - ReleaseResourcesIfPossible(); + releasedResources = ReleaseResourcesIfPossible(); + } + + if (releasedResources) + { + OnAfterReleaseResourcesUnlocked(); } if (deserializeException != null && !IsClient) diff --git a/src/csharp/Grpc.Core/Internal/AsyncCallServer.cs b/src/csharp/Grpc.Core/Internal/AsyncCallServer.cs index 11acb27533..0ceca4abb8 100644 --- a/src/csharp/Grpc.Core/Internal/AsyncCallServer.cs +++ b/src/csharp/Grpc.Core/Internal/AsyncCallServer.cs @@ -184,7 +184,7 @@ namespace Grpc.Core.Internal throw new InvalidOperationException("Call be only called for client calls"); } - protected override void OnAfterReleaseResources() + protected override void OnAfterReleaseResourcesLocked() { server.RemoveCallReference(this); } @@ -206,6 +206,7 @@ namespace Grpc.Core.Internal { // NOTE: because this event is a result of batch containing GRPC_OP_RECV_CLOSE_ON_SERVER, // success will be always set to true. + bool releasedResources; lock (myLock) { finished = true; @@ -217,7 +218,12 @@ namespace Grpc.Core.Internal streamingReadTcs = new TaskCompletionSource<TRequest>(); streamingReadTcs.SetResult(default(TRequest)); } - ReleaseResourcesIfPossible(); + releasedResources = ReleaseResourcesIfPossible(); + } + + if (releasedResources) + { + OnAfterReleaseResourcesUnlocked(); } if (cancelled) diff --git a/src/csharp/Grpc.Core/Internal/BatchContextSafeHandle.cs b/src/csharp/Grpc.Core/Internal/BatchContextSafeHandle.cs index 53a859d18f..085e7faf59 100644 --- a/src/csharp/Grpc.Core/Internal/BatchContextSafeHandle.cs +++ b/src/csharp/Grpc.Core/Internal/BatchContextSafeHandle.cs @@ -144,7 +144,7 @@ namespace Grpc.Core.Internal } catch (Exception e) { - Logger.Error(e, "Exception occured while invoking batch completion delegate."); + Logger.Error(e, "Exception occurred while invoking batch completion delegate."); } finally { diff --git a/src/csharp/Grpc.Core/Internal/GrpcThreadPool.cs b/src/csharp/Grpc.Core/Internal/GrpcThreadPool.cs index 8ddda9be5c..622cfde9e7 100644 --- a/src/csharp/Grpc.Core/Internal/GrpcThreadPool.cs +++ b/src/csharp/Grpc.Core/Internal/GrpcThreadPool.cs @@ -189,7 +189,7 @@ namespace Grpc.Core.Internal } catch (Exception e) { - Logger.Error(e, "Exception occured while extracting event from completion registry."); + Logger.Error(e, "Exception occurred while extracting event from completion registry."); } } } @@ -233,7 +233,7 @@ namespace Grpc.Core.Internal } catch (Exception e) { - Logger.Error(e, "Exception occured while invoking completion delegate"); + Logger.Error(e, "Exception occurred while invoking completion delegate"); } finally { diff --git a/src/csharp/Grpc.Core/Internal/NativeMetadataCredentialsPlugin.cs b/src/csharp/Grpc.Core/Internal/NativeMetadataCredentialsPlugin.cs index 4d695e8850..faeb51e6f7 100644 --- a/src/csharp/Grpc.Core/Internal/NativeMetadataCredentialsPlugin.cs +++ b/src/csharp/Grpc.Core/Internal/NativeMetadataCredentialsPlugin.cs @@ -68,8 +68,8 @@ namespace Grpc.Core.Internal } catch (Exception e) { - Native.grpcsharp_metadata_credentials_notify_from_plugin(callbackPtr, userDataPtr, MetadataArraySafeHandle.Create(Metadata.Empty), StatusCode.Unknown, GetMetadataExceptionStatusMsg); - Logger.Error(e, GetMetadataExceptionLogMsg); + // eat the exception, we must not throw when inside callback from native code. + Logger.Error(e, "Exception occurred while invoking native metadata interceptor handler."); } } @@ -87,7 +87,8 @@ namespace Grpc.Core.Internal } catch (Exception e) { - Native.grpcsharp_metadata_credentials_notify_from_plugin(callbackPtr, userDataPtr, MetadataArraySafeHandle.Create(Metadata.Empty), StatusCode.Unknown, GetMetadataExceptionStatusMsg); + string detail = GetMetadataExceptionStatusMsg + " " + e.ToString(); + Native.grpcsharp_metadata_credentials_notify_from_plugin(callbackPtr, userDataPtr, MetadataArraySafeHandle.Create(Metadata.Empty), StatusCode.Unknown, detail); Logger.Error(e, GetMetadataExceptionLogMsg); } } diff --git a/src/csharp/Grpc.Core/Internal/RequestCallContextSafeHandle.cs b/src/csharp/Grpc.Core/Internal/RequestCallContextSafeHandle.cs index ebc2d6d8d6..24fde75e5a 100644 --- a/src/csharp/Grpc.Core/Internal/RequestCallContextSafeHandle.cs +++ b/src/csharp/Grpc.Core/Internal/RequestCallContextSafeHandle.cs @@ -112,7 +112,7 @@ namespace Grpc.Core.Internal } catch (Exception e) { - Logger.Error(e, "Exception occured while invoking request call completion delegate."); + Logger.Error(e, "Exception occurred while invoking request call completion delegate."); } finally { diff --git a/src/csharp/Grpc.Core/Metadata.cs b/src/csharp/Grpc.Core/Metadata.cs index 0e4456278c..281952d6d4 100644 --- a/src/csharp/Grpc.Core/Metadata.cs +++ b/src/csharp/Grpc.Core/Metadata.cs @@ -225,7 +225,7 @@ namespace Grpc.Core /// </summary> public class Entry { - private static readonly Regex ValidKeyRegex = new Regex("^[a-z0-9_-]+$"); + private static readonly Regex ValidKeyRegex = new Regex("^[.a-z0-9_-]+$"); readonly string key; readonly string value; @@ -360,7 +360,7 @@ namespace Grpc.Core { var normalized = GrpcPreconditions.CheckNotNull(key, "key").ToLowerInvariant(); GrpcPreconditions.CheckArgument(ValidKeyRegex.IsMatch(normalized), - "Metadata entry key not valid. Keys can only contain lowercase alphanumeric characters, underscores and hyphens."); + "Metadata entry key not valid. Keys can only contain lowercase alphanumeric characters, underscores, hyphens and dots."); return normalized; } diff --git a/src/csharp/Grpc.Core/RpcException.cs b/src/csharp/Grpc.Core/RpcException.cs index 94429d74ce..ff89897565 100644 --- a/src/csharp/Grpc.Core/RpcException.cs +++ b/src/csharp/Grpc.Core/RpcException.cs @@ -33,10 +33,8 @@ namespace Grpc.Core /// Creates a new <c>RpcException</c> associated with given status. /// </summary> /// <param name="status">Resulting status of a call.</param> - public RpcException(Status status) : base(status.ToString()) + public RpcException(Status status) : this(status, Metadata.Empty, status.ToString()) { - this.status = status; - this.trailers = Metadata.Empty; } /// <summary> @@ -44,10 +42,8 @@ namespace Grpc.Core /// </summary> /// <param name="status">Resulting status of a call.</param> /// <param name="message">The exception message.</param> - public RpcException(Status status, string message) : base(message) + public RpcException(Status status, string message) : this(status, Metadata.Empty, message) { - this.status = status; - this.trailers = Metadata.Empty; } /// <summary> @@ -55,7 +51,17 @@ namespace Grpc.Core /// </summary> /// <param name="status">Resulting status of a call.</param> /// <param name="trailers">Response trailing metadata.</param> - public RpcException(Status status, Metadata trailers) : base(status.ToString()) + public RpcException(Status status, Metadata trailers) : this(status, trailers, status.ToString()) + { + } + + /// <summary> + /// Creates a new <c>RpcException</c> associated with given status, message and trailing response metadata. + /// </summary> + /// <param name="status">Resulting status of a call.</param> + /// <param name="trailers">Response trailing metadata.</param> + /// <param name="message">The exception message.</param> + public RpcException(Status status, Metadata trailers, string message) : base(message) { this.status = status; this.trailers = GrpcPreconditions.CheckNotNull(trailers); diff --git a/src/csharp/Grpc.Core/Version.csproj.include b/src/csharp/Grpc.Core/Version.csproj.include index 45bd8ebd85..18515ea1e8 100755 --- a/src/csharp/Grpc.Core/Version.csproj.include +++ b/src/csharp/Grpc.Core/Version.csproj.include @@ -1,7 +1,7 @@ <!-- This file is generated --> <Project> <PropertyGroup> - <GrpcCsharpVersion>1.15.0-dev</GrpcCsharpVersion> + <GrpcCsharpVersion>1.16.0-dev</GrpcCsharpVersion> <GoogleProtobufVersion>3.6.1</GoogleProtobufVersion> </PropertyGroup> </Project> diff --git a/src/csharp/Grpc.Core/VersionInfo.cs b/src/csharp/Grpc.Core/VersionInfo.cs index 295e0f2577..55d09dda7a 100644 --- a/src/csharp/Grpc.Core/VersionInfo.cs +++ b/src/csharp/Grpc.Core/VersionInfo.cs @@ -33,11 +33,11 @@ namespace Grpc.Core /// <summary> /// Current <c>AssemblyFileVersion</c> of gRPC C# assemblies /// </summary> - public const string CurrentAssemblyFileVersion = "1.15.0.0"; + public const string CurrentAssemblyFileVersion = "1.16.0.0"; /// <summary> /// Current version of gRPC C# /// </summary> - public const string CurrentVersion = "1.15.0-dev"; + public const string CurrentVersion = "1.16.0-dev"; } } diff --git a/src/csharp/Grpc.IntegrationTesting/MetadataCredentialsTest.cs b/src/csharp/Grpc.IntegrationTesting/MetadataCredentialsTest.cs index c83ccd2612..40447854f4 100644 --- a/src/csharp/Grpc.IntegrationTesting/MetadataCredentialsTest.cs +++ b/src/csharp/Grpc.IntegrationTesting/MetadataCredentialsTest.cs @@ -153,9 +153,10 @@ namespace Grpc.IntegrationTesting [Test] public void MetadataCredentials_InterceptorThrows() { + var authInterceptorExceptionMessage = "Auth interceptor throws"; var callCredentials = CallCredentials.FromInterceptor(new AsyncAuthInterceptor((context, metadata) => { - throw new Exception("Auth interceptor throws"); + throw new Exception(authInterceptorExceptionMessage); })); var channelCredentials = ChannelCredentials.Create(TestCredentials.CreateSslCredentials(), callCredentials); channel = new Channel(Host, server.Ports.Single().BoundPort, channelCredentials, options); @@ -163,6 +164,7 @@ namespace Grpc.IntegrationTesting var ex = Assert.Throws<RpcException>(() => client.UnaryCall(new SimpleRequest { })); Assert.AreEqual(StatusCode.Unavailable, ex.Status.StatusCode); + StringAssert.Contains(authInterceptorExceptionMessage, ex.Status.Detail); } private class FakeTestService : TestService.TestServiceBase diff --git a/src/csharp/build_packages_dotnetcli.bat b/src/csharp/build_packages_dotnetcli.bat index 8f38f0aa20..24d016104c 100755 --- a/src/csharp/build_packages_dotnetcli.bat +++ b/src/csharp/build_packages_dotnetcli.bat @@ -13,7 +13,7 @@ @rem limitations under the License. @rem Current package versions -set VERSION=1.15.0-dev +set VERSION=1.16.0-dev @rem Adjust the location of nuget.exe set NUGET=C:\nuget\nuget.exe diff --git a/src/csharp/build_unitypackage.bat b/src/csharp/build_unitypackage.bat index 9c53114b84..4e7ac4e414 100644 --- a/src/csharp/build_unitypackage.bat +++ b/src/csharp/build_unitypackage.bat @@ -13,7 +13,7 @@ @rem limitations under the License. @rem Current package versions -set VERSION=1.15.0-dev +set VERSION=1.16.0-dev @rem Adjust the location of nuget.exe set NUGET=C:\nuget\nuget.exe |