From f44f8df28cc984fbd8debd85a7e3f360233919b1 Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Fri, 6 Mar 2020 10:20:00 +1300 Subject: [PATCH 1/4] Remove task allocation in unary call handler --- .../DefaultCoreConfig.cs | 2 +- src/Shared/Server/UnaryServerMethodInvoker.cs | 65 +++++++++++++++++-- 2 files changed, 61 insertions(+), 6 deletions(-) diff --git a/perf/Grpc.AspNetCore.Microbenchmarks/DefaultCoreConfig.cs b/perf/Grpc.AspNetCore.Microbenchmarks/DefaultCoreConfig.cs index fccd0912d..4646855ca 100644 --- a/perf/Grpc.AspNetCore.Microbenchmarks/DefaultCoreConfig.cs +++ b/perf/Grpc.AspNetCore.Microbenchmarks/DefaultCoreConfig.cs @@ -44,7 +44,7 @@ public DefaultCoreConfig() Add(JitOptimizationsValidator.FailOnError); Add(Job.Core - .With(CsProjCoreToolchain.From(new NetCoreAppSettings("netcoreapp3.0", null, ".NET Core 3.0"))) + .With(CsProjCoreToolchain.From(new NetCoreAppSettings("netcoreapp3.1", null, ".NET Core 3.1"))) .With(new GcMode { Server = true }) .With(RunStrategy.Throughput)); } diff --git a/src/Shared/Server/UnaryServerMethodInvoker.cs b/src/Shared/Server/UnaryServerMethodInvoker.cs index 5b817b1ed..5d00bc32d 100644 --- a/src/Shared/Server/UnaryServerMethodInvoker.cs +++ b/src/Shared/Server/UnaryServerMethodInvoker.cs @@ -16,6 +16,8 @@ #endregion +using System; +using System.Runtime.ExceptionServices; using System.Threading.Tasks; using Grpc.AspNetCore.Server; using Grpc.AspNetCore.Server.Model; @@ -86,33 +88,86 @@ private async Task ResolvedInterceptorInvoker(TRequest resolvedReques /// The message. /// A that represents the asynchronous method. The /// property returns the message. - public async Task Invoke(HttpContext httpContext, ServerCallContext serverCallContext, TRequest request) + public Task Invoke(HttpContext httpContext, ServerCallContext serverCallContext, TRequest request) { if (_pipelineInvoker == null) { GrpcActivatorHandle serviceHandle = default; + Task? invokerTask = null; try { serviceHandle = ServiceActivator.Create(httpContext.RequestServices); - return await _invoker( + invokerTask = _invoker( serviceHandle.Instance, request, serverCallContext); } - finally + catch (Exception ex) { + // Invoker calls user code and instead of returning a faulted task + // it may directly throw an exception. Catch and handle converting the + // exception into a task. if (serviceHandle.Instance != null) { - await ServiceActivator.ReleaseAsync(serviceHandle); + var releaseTask = ServiceActivator.ReleaseAsync(serviceHandle); + if (!releaseTask.IsCompletedSuccessfully) + { + return AwaitServiceReleaseAndThrow(serviceHandle, ex); + } } + + return Task.FromException(ex); + } + + if (invokerTask.IsCompletedSuccessfully && serviceHandle.Instance != null) + { + var releaseTask = ServiceActivator.ReleaseAsync(serviceHandle); + if (!releaseTask.IsCompletedSuccessfully) + { + return AwaitServiceReleaseAndReturn(serviceHandle, invokerTask.Result); + } + + return invokerTask; } + + return AwaitInvoker(serviceHandle, invokerTask); } else { - return await _pipelineInvoker( + return _pipelineInvoker( request, serverCallContext); } } + + private async Task AwaitInvoker(GrpcActivatorHandle serviceHandle, Task invokerTask) + { + try + { + return await invokerTask; + } + finally + { + if (serviceHandle.Instance != null) + { + await ServiceActivator.ReleaseAsync(serviceHandle); + } + } + } + + private async Task AwaitServiceReleaseAndThrow(GrpcActivatorHandle serviceHandle, Exception ex) + { + await ServiceActivator.ReleaseAsync(serviceHandle); + ExceptionDispatchInfo.Capture(ex).Throw(); + + // Should never reach here + return null; + } + + private async Task AwaitServiceReleaseAndReturn(GrpcActivatorHandle serviceHandle, TResponse invokerResult) + { + await ServiceActivator.ReleaseAsync(serviceHandle); + return invokerResult; + } } } From e05e37b8835b4fed6f441dd2e1cb2d9ef42bbb2a Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Fri, 6 Mar 2020 11:41:04 +1300 Subject: [PATCH 2/4] Tests --- src/Shared/Server/UnaryServerMethodInvoker.cs | 22 +-- .../TestObjects/TestGrpcServiceActivator.cs | 3 + .../UnaryServerCallHandlerTests.cs | 163 ++++++++++++++++++ 3 files changed, 177 insertions(+), 11 deletions(-) create mode 100644 test/Grpc.AspNetCore.Server.Tests/UnaryServerCallHandlerTests.cs diff --git a/src/Shared/Server/UnaryServerMethodInvoker.cs b/src/Shared/Server/UnaryServerMethodInvoker.cs index 5d00bc32d..505402712 100644 --- a/src/Shared/Server/UnaryServerMethodInvoker.cs +++ b/src/Shared/Server/UnaryServerMethodInvoker.cs @@ -104,15 +104,15 @@ public Task Invoke(HttpContext httpContext, ServerCallContext serverC } catch (Exception ex) { - // Invoker calls user code and instead of returning a faulted task - // it may directly throw an exception. Catch and handle converting the - // exception into a task. + // Invoker calls user code. User code may throw an exception instead + // of a faulted task. We need to catch the exception, ensure cleanup + // runs and convert exception into a faulted task. if (serviceHandle.Instance != null) { var releaseTask = ServiceActivator.ReleaseAsync(serviceHandle); if (!releaseTask.IsCompletedSuccessfully) { - return AwaitServiceReleaseAndThrow(serviceHandle, ex); + return AwaitServiceReleaseAndThrow(releaseTask, ExceptionDispatchInfo.Capture(ex)); } } @@ -124,13 +124,13 @@ public Task Invoke(HttpContext httpContext, ServerCallContext serverC var releaseTask = ServiceActivator.ReleaseAsync(serviceHandle); if (!releaseTask.IsCompletedSuccessfully) { - return AwaitServiceReleaseAndReturn(serviceHandle, invokerTask.Result); + return AwaitServiceReleaseAndReturn(invokerTask.Result, serviceHandle); } return invokerTask; } - return AwaitInvoker(serviceHandle, invokerTask); + return AwaitInvoker(invokerTask, serviceHandle); } else { @@ -140,7 +140,7 @@ public Task Invoke(HttpContext httpContext, ServerCallContext serverC } } - private async Task AwaitInvoker(GrpcActivatorHandle serviceHandle, Task invokerTask) + private async Task AwaitInvoker(Task invokerTask, GrpcActivatorHandle serviceHandle) { try { @@ -155,16 +155,16 @@ private async Task AwaitInvoker(GrpcActivatorHandle service } } - private async Task AwaitServiceReleaseAndThrow(GrpcActivatorHandle serviceHandle, Exception ex) + private async Task AwaitServiceReleaseAndThrow(ValueTask releaseTask, ExceptionDispatchInfo ex) { - await ServiceActivator.ReleaseAsync(serviceHandle); - ExceptionDispatchInfo.Capture(ex).Throw(); + await releaseTask; + ex.Throw(); // Should never reach here return null; } - private async Task AwaitServiceReleaseAndReturn(GrpcActivatorHandle serviceHandle, TResponse invokerResult) + private async Task AwaitServiceReleaseAndReturn(TResponse invokerResult, GrpcActivatorHandle serviceHandle) { await ServiceActivator.ReleaseAsync(serviceHandle); return invokerResult; diff --git a/test/Grpc.AspNetCore.Server.Tests/TestObjects/TestGrpcServiceActivator.cs b/test/Grpc.AspNetCore.Server.Tests/TestObjects/TestGrpcServiceActivator.cs index 9a7866ab0..3559317b3 100644 --- a/test/Grpc.AspNetCore.Server.Tests/TestObjects/TestGrpcServiceActivator.cs +++ b/test/Grpc.AspNetCore.Server.Tests/TestObjects/TestGrpcServiceActivator.cs @@ -23,6 +23,8 @@ namespace Grpc.AspNetCore.Server.Tests.TestObjects { internal class TestGrpcServiceActivator : IGrpcServiceActivator where TGrpcService : class, new() { + public bool Released { get; private set; } + public GrpcActivatorHandle Create(IServiceProvider serviceProvider) { return new GrpcActivatorHandle(new TGrpcService(), false, null); @@ -30,6 +32,7 @@ public GrpcActivatorHandle Create(IServiceProvider serviceProvider public ValueTask ReleaseAsync(GrpcActivatorHandle service) { + Released = true; return default; } } diff --git a/test/Grpc.AspNetCore.Server.Tests/UnaryServerCallHandlerTests.cs b/test/Grpc.AspNetCore.Server.Tests/UnaryServerCallHandlerTests.cs new file mode 100644 index 000000000..38b4c0f53 --- /dev/null +++ b/test/Grpc.AspNetCore.Server.Tests/UnaryServerCallHandlerTests.cs @@ -0,0 +1,163 @@ +#region Copyright notice and license + +// Copyright 2019 The gRPC Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#endregion + +using System; +using System.Threading.Tasks; +using Grpc.AspNetCore.Server.Tests.TestObjects; +using Grpc.Core; +using Grpc.Shared.Server; +using Grpc.Tests.Shared; +using NUnit.Framework; + +namespace Grpc.AspNetCore.Server.Tests +{ + [TestFixture] + public class UnaryServerCallHandlerTests + { + private static readonly Marshaller _marshaller = new Marshaller((message, context) => { context.Complete(Array.Empty()); }, context => new TestMessage()); + + [Test] + public void Invoke_ThrowException_ReleaseCalledAndErrorThrown() + { + // Arrange + var serviceActivator = new TestGrpcServiceActivator(); + var ex = new Exception("Exception!"); + var invoker = new UnaryServerMethodInvoker( + (service, reader, context) => throw ex, + new Method(MethodType.Unary, "test", "test", _marshaller, _marshaller), + HttpContextServerCallContextHelper.CreateMethodOptions(), + serviceActivator); + var httpContext = HttpContextHelpers.CreateContext(); + + // Act + var task = invoker.Invoke(httpContext, HttpContextServerCallContextHelper.CreateServerCallContext(), new TestMessage()); + + // Assert + Assert.True(serviceActivator.Released); + Assert.True(task.IsFaulted); + Assert.AreEqual(ex, task.Exception!.InnerException); + } + + [Test] + public async Task Invoke_ThrowExceptionAwaitedRelease_ReleaseCalledAndErrorThrown() + { + // Arrange + var releaseTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var serviceActivator = new TcsGrpcServiceActivator(releaseTcs); + var thrownException = new Exception("Exception!"); + var invoker = new UnaryServerMethodInvoker( + (service, reader, context) => throw thrownException, + new Method(MethodType.Unary, "test", "test", _marshaller, _marshaller), + HttpContextServerCallContextHelper.CreateMethodOptions(), + serviceActivator); + var httpContext = HttpContextHelpers.CreateContext(); + + // Act + var task = invoker.Invoke(httpContext, HttpContextServerCallContextHelper.CreateServerCallContext(), new TestMessage()); + Assert.False(task.IsCompleted); + + //await Task.Delay(1000); + releaseTcs.SetResult(null); + + try + { + await task; + Assert.Fail(); + } + catch (Exception ex) + { + // Assert + Assert.True(serviceActivator.Released); + Assert.AreEqual(thrownException, ex); + } + } + + [Test] + public async Task Invoke_SuccessAwaitedRelease_ReleaseCalled() + { + // Arrange + var releaseTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var serviceActivator = new TcsGrpcServiceActivator(releaseTcs); + var invoker = new UnaryServerMethodInvoker( + (service, reader, context) => Task.FromResult(new TestMessage()), + new Method(MethodType.Unary, "test", "test", _marshaller, _marshaller), + HttpContextServerCallContextHelper.CreateMethodOptions(), + serviceActivator); + var httpContext = HttpContextHelpers.CreateContext(); + + // Act + var task = invoker.Invoke(httpContext, HttpContextServerCallContextHelper.CreateServerCallContext(), new TestMessage()); + Assert.False(task.IsCompleted); + + releaseTcs.SetResult(null); + await task; + + // Assert + Assert.True(serviceActivator.Released); + } + + [Test] + public async Task Invoke_AwaitedSuccess_ReleaseCalled() + { + // Arrange + var methodTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var methodResult = new TestMessage(); + var serviceActivator = new TestGrpcServiceActivator(); + var invoker = new UnaryServerMethodInvoker( + (service, reader, context) => methodTcs.Task, + new Method(MethodType.Unary, "test", "test", _marshaller, _marshaller), + HttpContextServerCallContextHelper.CreateMethodOptions(), + serviceActivator); + var httpContext = HttpContextHelpers.CreateContext(); + + // Act + var task = invoker.Invoke(httpContext, HttpContextServerCallContextHelper.CreateServerCallContext(), new TestMessage()); + Assert.False(task.IsCompleted); + + methodTcs.SetResult(methodResult); + var awaitedResult = await task; + + // Assert + Assert.AreEqual(methodResult, awaitedResult); + Assert.True(serviceActivator.Released); + } + + private class TcsGrpcServiceActivator : IGrpcServiceActivator where TGrpcService : class, new() + { + private readonly TaskCompletionSource _tcs; + + public bool Released { get; private set; } + + public TcsGrpcServiceActivator(TaskCompletionSource tcs) + { + _tcs = tcs; + } + + public GrpcActivatorHandle Create(IServiceProvider serviceProvider) + { + return new GrpcActivatorHandle(new TGrpcService(), false, null); + } + + public ValueTask ReleaseAsync(GrpcActivatorHandle service) + { + Released = true; + return new ValueTask(_tcs.Task); + } + } + } +} From 3ed819c7f96d263797256043f0947d8e960dc4f4 Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Fri, 6 Mar 2020 11:49:09 +1300 Subject: [PATCH 3/4] Clean up --- src/Shared/Server/UnaryServerMethodInvoker.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/Shared/Server/UnaryServerMethodInvoker.cs b/src/Shared/Server/UnaryServerMethodInvoker.cs index 505402712..f39009f90 100644 --- a/src/Shared/Server/UnaryServerMethodInvoker.cs +++ b/src/Shared/Server/UnaryServerMethodInvoker.cs @@ -112,7 +112,10 @@ public Task Invoke(HttpContext httpContext, ServerCallContext serverC var releaseTask = ServiceActivator.ReleaseAsync(serviceHandle); if (!releaseTask.IsCompletedSuccessfully) { - return AwaitServiceReleaseAndThrow(releaseTask, ExceptionDispatchInfo.Capture(ex)); + // Capture the current exception state so we can rethrow it after awaiting + // with the same stack trace. + var exceptionDispatchInfo = ExceptionDispatchInfo.Capture(ex); + return AwaitServiceReleaseAndThrow(releaseTask, exceptionDispatchInfo); } } From 3c1f5ea9122f4c9ab2ba9b41f0f148d55112535e Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Fri, 6 Mar 2020 13:24:02 +1300 Subject: [PATCH 4/4] Update test/Grpc.AspNetCore.Server.Tests/UnaryServerCallHandlerTests.cs --- test/Grpc.AspNetCore.Server.Tests/UnaryServerCallHandlerTests.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/test/Grpc.AspNetCore.Server.Tests/UnaryServerCallHandlerTests.cs b/test/Grpc.AspNetCore.Server.Tests/UnaryServerCallHandlerTests.cs index 38b4c0f53..16137e2c1 100644 --- a/test/Grpc.AspNetCore.Server.Tests/UnaryServerCallHandlerTests.cs +++ b/test/Grpc.AspNetCore.Server.Tests/UnaryServerCallHandlerTests.cs @@ -71,7 +71,6 @@ public async Task Invoke_ThrowExceptionAwaitedRelease_ReleaseCalledAndErrorThrow var task = invoker.Invoke(httpContext, HttpContextServerCallContextHelper.CreateServerCallContext(), new TestMessage()); Assert.False(task.IsCompleted); - //await Task.Delay(1000); releaseTcs.SetResult(null); try