From dbb4c0f52d87a141a821b5b7ff4212411bed9fa3 Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Thu, 20 Apr 2023 15:49:23 +0800 Subject: [PATCH 1/4] Disable request timeout for streaming methods --- .../ClientStreamingServerCallHandler.cs | 7 ++- .../DuplexStreamingServerCallHandler.cs | 7 ++- .../CallHandlers/ServerCallHandlerBase.cs | 28 ++++++++- .../ServerStreamingServerCallHandler.cs | 7 ++- .../CallHandlerTests.cs | 58 ++++++++++++++++++- 5 files changed, 100 insertions(+), 7 deletions(-) diff --git a/src/Grpc.AspNetCore.Server/Internal/CallHandlers/ClientStreamingServerCallHandler.cs b/src/Grpc.AspNetCore.Server/Internal/CallHandlers/ClientStreamingServerCallHandler.cs index d39b11aee..61655c71a 100644 --- a/src/Grpc.AspNetCore.Server/Internal/CallHandlers/ClientStreamingServerCallHandler.cs +++ b/src/Grpc.AspNetCore.Server/Internal/CallHandlers/ClientStreamingServerCallHandler.cs @@ -1,4 +1,4 @@ -#region Copyright notice and license +#region Copyright notice and license // Copyright 2019 The gRPC Authors // @@ -45,8 +45,11 @@ public ClientStreamingServerCallHandler( protected override async Task HandleCallAsyncCore(HttpContext httpContext, HttpContextServerCallContext serverCallContext) { - // Disable request body data rate for client streaming + // Disable certain features for client streaming methods. DisableMinRequestBodyDataRateAndMaxRequestBodySize(httpContext); +#if NET8_0_OR_GREATER + DisableRequestTimeout(httpContext); +#endif TResponse? response; diff --git a/src/Grpc.AspNetCore.Server/Internal/CallHandlers/DuplexStreamingServerCallHandler.cs b/src/Grpc.AspNetCore.Server/Internal/CallHandlers/DuplexStreamingServerCallHandler.cs index b9d44a6df..9513ab1b6 100644 --- a/src/Grpc.AspNetCore.Server/Internal/CallHandlers/DuplexStreamingServerCallHandler.cs +++ b/src/Grpc.AspNetCore.Server/Internal/CallHandlers/DuplexStreamingServerCallHandler.cs @@ -1,4 +1,4 @@ -#region Copyright notice and license +#region Copyright notice and license // Copyright 2019 The gRPC Authors // @@ -44,8 +44,11 @@ public DuplexStreamingServerCallHandler( protected override async Task HandleCallAsyncCore(HttpContext httpContext, HttpContextServerCallContext serverCallContext) { - // Disable request body data rate for client streaming + // Disable certain features for duplex streaming methods. DisableMinRequestBodyDataRateAndMaxRequestBodySize(httpContext); +#if NET8_0_OR_GREATER + DisableRequestTimeout(httpContext); +#endif var streamReader = new HttpContextStreamReader(serverCallContext, MethodInvoker.Method.RequestMarshaller.ContextualDeserializer); var streamWriter = new HttpContextStreamWriter(serverCallContext, MethodInvoker.Method.ResponseMarshaller.ContextualSerializer); diff --git a/src/Grpc.AspNetCore.Server/Internal/CallHandlers/ServerCallHandlerBase.cs b/src/Grpc.AspNetCore.Server/Internal/CallHandlers/ServerCallHandlerBase.cs index 3078de5d7..3596d3131 100644 --- a/src/Grpc.AspNetCore.Server/Internal/CallHandlers/ServerCallHandlerBase.cs +++ b/src/Grpc.AspNetCore.Server/Internal/CallHandlers/ServerCallHandlerBase.cs @@ -1,4 +1,4 @@ -#region Copyright notice and license +#region Copyright notice and license // Copyright 2019 The gRPC Authors // @@ -21,6 +21,9 @@ using Grpc.Shared.Server; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Http.Features; +#if NET8_0_OR_GREATER +using Microsoft.AspNetCore.Http.Timeouts; +#endif using Microsoft.AspNetCore.Server.Kestrel.Core.Features; using Microsoft.Extensions.Logging; using Microsoft.Net.Http.Headers; @@ -134,6 +137,29 @@ protected void DisableMinRequestBodyDataRateAndMaxRequestBodySize(HttpContext ht } } +#if NET8_0_OR_GREATER + protected void DisableRequestTimeout(HttpContext httpContext) + { + // Disable global request timeout on streaming methods. + var requestTimeoutFeature = httpContext.Features.Get(); + if (requestTimeoutFeature is not null) + { + // Don't disable if the endpoint has explicit timeout metadata. + var endpoint = httpContext.GetEndpoint(); + if (endpoint is not null) + { + if (endpoint.Metadata.GetMetadata() is not null || + endpoint.Metadata.GetMetadata() is not null) + { + return; + } + } + + requestTimeoutFeature.DisableTimeout(); + } + } +#endif + private Task ProcessNonHttp2Request(HttpContext httpContext) { GrpcServerLog.UnsupportedRequestProtocol(Logger, httpContext.Request.Protocol); diff --git a/src/Grpc.AspNetCore.Server/Internal/CallHandlers/ServerStreamingServerCallHandler.cs b/src/Grpc.AspNetCore.Server/Internal/CallHandlers/ServerStreamingServerCallHandler.cs index 17a1d9253..e29c58be5 100644 --- a/src/Grpc.AspNetCore.Server/Internal/CallHandlers/ServerStreamingServerCallHandler.cs +++ b/src/Grpc.AspNetCore.Server/Internal/CallHandlers/ServerStreamingServerCallHandler.cs @@ -1,4 +1,4 @@ -#region Copyright notice and license +#region Copyright notice and license // Copyright 2019 The gRPC Authors // @@ -44,6 +44,11 @@ public ServerStreamingServerCallHandler( protected override async Task HandleCallAsyncCore(HttpContext httpContext, HttpContextServerCallContext serverCallContext) { + // Disable certain features for server streaming methods. +#if NET8_0_OR_GREATER + DisableRequestTimeout(httpContext); +#endif + // Decode request var request = await httpContext.Request.BodyReader.ReadSingleMessageAsync(serverCallContext, MethodInvoker.Method.RequestMarshaller.ContextualDeserializer); diff --git a/test/Grpc.AspNetCore.Server.Tests/CallHandlerTests.cs b/test/Grpc.AspNetCore.Server.Tests/CallHandlerTests.cs index 1a55f3ac3..d4814d11d 100644 --- a/test/Grpc.AspNetCore.Server.Tests/CallHandlerTests.cs +++ b/test/Grpc.AspNetCore.Server.Tests/CallHandlerTests.cs @@ -1,4 +1,4 @@ -#region Copyright notice and license +#region Copyright notice and license // Copyright 2019 The gRPC Authors // @@ -22,7 +22,11 @@ using Grpc.Core; using Grpc.Shared.Server; using Grpc.Tests.Shared; +using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Http.Features; +#if NET8_0_OR_GREATER +using Microsoft.AspNetCore.Http.Timeouts; +#endif using Microsoft.AspNetCore.Server.Kestrel.Core.Features; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; @@ -167,6 +171,58 @@ public async Task ProtocolValidation_IISHttp2Protocol_Success() } #endif +#if NET8_0_OR_GREATER + [TestCase(MethodType.Unary, false)] + [TestCase(MethodType.ClientStreaming, true)] + [TestCase(MethodType.ServerStreaming, true)] + [TestCase(MethodType.DuplexStreaming, true)] + public async Task RequestTimeoutFeature_Global_DisableWhenStreaming(MethodType methodType, bool expectedTimeoutDisabled) + { + // Arrange + var timeoutFeature = new TestHttpRequestTimeoutFeature(); + var httpContext = HttpContextHelpers.CreateContext(); + httpContext.Features.Set(timeoutFeature); + var call = CreateHandler(methodType); + + // Act + await call.HandleCallAsync(httpContext).DefaultTimeout(); + + // Assert + Assert.AreEqual(expectedTimeoutDisabled, timeoutFeature.TimeoutDisabled); + } + + [TestCase(MethodType.Unary, false)] + [TestCase(MethodType.ClientStreaming, false)] + [TestCase(MethodType.ServerStreaming, false)] + [TestCase(MethodType.DuplexStreaming, false)] + public async Task RequestTimeoutFeature_EndpointMetadata_DisableWhenStreaming(MethodType methodType, bool expectedTimeoutDisabled) + { + // Arrange + var timeoutFeature = new TestHttpRequestTimeoutFeature(); + var httpContext = HttpContextHelpers.CreateContext(); + httpContext.SetEndpoint(new Endpoint(c => Task.CompletedTask, new EndpointMetadataCollection(new RequestTimeoutAttribute(100)), "Test endpoint")); + httpContext.Features.Set(timeoutFeature); + var call = CreateHandler(methodType); + + // Act + await call.HandleCallAsync(httpContext).DefaultTimeout(); + + // Assert + Assert.AreEqual(expectedTimeoutDisabled, timeoutFeature.TimeoutDisabled); + } + + private sealed class TestHttpRequestTimeoutFeature : IHttpRequestTimeoutFeature + { + public bool TimeoutDisabled { get; private set; } + public CancellationToken RequestTimeoutToken { get; } + + public void DisableTimeout() + { + TimeoutDisabled = true; + } + } +#endif + [Test] public async Task StatusDebugException_ErrorInHandler_SetInDebugException() { From b483c6b6a9c3ea9ea9dd1676c9ef36103b4e03d1 Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Thu, 20 Apr 2023 22:14:10 +0800 Subject: [PATCH 2/4] Test STJ issue --- examples/Container/Backend/Dockerfile | 4 ++-- examples/Container/Frontend/Dockerfile | 4 ++-- testassets/InteropTestsGrpcWebWebsite/Dockerfile | 4 ++-- testassets/InteropTestsWebsite/Dockerfile | 4 ++-- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/examples/Container/Backend/Dockerfile b/examples/Container/Backend/Dockerfile index 19b794071..afa869b4e 100644 --- a/examples/Container/Backend/Dockerfile +++ b/examples/Container/Backend/Dockerfile @@ -1,4 +1,4 @@ -FROM mcr.microsoft.com/dotnet/nightly/sdk:8.0-preview AS build-env +FROM mcr.microsoft.com/dotnet/sdk:8.0-preview AS build-env WORKDIR /app # Copy everything @@ -8,7 +8,7 @@ RUN dotnet restore examples/Container/Backend RUN dotnet publish examples/Container/Backend -c Release -o out # Build runtime image -FROM mcr.microsoft.com/dotnet/nightly/aspnet:8.0-preview +FROM mcr.microsoft.com/dotnet/aspnet:8.0-preview WORKDIR /app COPY --from=build-env /app/out . ENTRYPOINT ["dotnet", "Backend.dll"] \ No newline at end of file diff --git a/examples/Container/Frontend/Dockerfile b/examples/Container/Frontend/Dockerfile index 09ebb01b3..44634948c 100644 --- a/examples/Container/Frontend/Dockerfile +++ b/examples/Container/Frontend/Dockerfile @@ -1,4 +1,4 @@ -FROM mcr.microsoft.com/dotnet/nightly/sdk:8.0-preview AS build-env +FROM mcr.microsoft.com/dotnet/sdk:8.0-preview AS build-env WORKDIR /app # Copy everything @@ -8,7 +8,7 @@ RUN dotnet restore examples/Container/Frontend RUN dotnet publish examples/Container/Frontend -c Release -o out # Build runtime image -FROM mcr.microsoft.com/dotnet/nightly/aspnet:8.0-preview +FROM mcr.microsoft.com/dotnet/aspnet:8.0-preview WORKDIR /app COPY --from=build-env /app/out . ENTRYPOINT ["dotnet", "Frontend.dll"] \ No newline at end of file diff --git a/testassets/InteropTestsGrpcWebWebsite/Dockerfile b/testassets/InteropTestsGrpcWebWebsite/Dockerfile index cd82ea52b..0115da9cb 100644 --- a/testassets/InteropTestsGrpcWebWebsite/Dockerfile +++ b/testassets/InteropTestsGrpcWebWebsite/Dockerfile @@ -1,4 +1,4 @@ -FROM mcr.microsoft.com/dotnet/nightly/sdk:8.0-preview AS build-env +FROM mcr.microsoft.com/dotnet/sdk:8.0-preview AS build-env WORKDIR /app # Copy everything @@ -8,7 +8,7 @@ RUN dotnet restore testassets/InteropTestsGrpcWebWebsite RUN dotnet publish testassets/InteropTestsGrpcWebWebsite -c Release -o out # Build runtime image -FROM mcr.microsoft.com/dotnet/nightly/aspnet:8.0-preview +FROM mcr.microsoft.com/dotnet/aspnet:8.0-preview WORKDIR /app COPY --from=build-env /app/out . ENTRYPOINT ["dotnet", "InteropTestsGrpcWebWebsite.dll", "--urls", "http://+:80"] diff --git a/testassets/InteropTestsWebsite/Dockerfile b/testassets/InteropTestsWebsite/Dockerfile index b60d6d6df..81e4333b8 100644 --- a/testassets/InteropTestsWebsite/Dockerfile +++ b/testassets/InteropTestsWebsite/Dockerfile @@ -1,4 +1,4 @@ -FROM mcr.microsoft.com/dotnet/nightly/sdk:8.0-preview AS build-env +FROM mcr.microsoft.com/dotnet/sdk:8.0-preview AS build-env WORKDIR /app # Copy everything @@ -8,7 +8,7 @@ RUN dotnet restore testassets/InteropTestsWebsite RUN dotnet publish testassets/InteropTestsWebsite --framework net8.0 -c Release -o out -p:LatestFramework=true # Build runtime image -FROM mcr.microsoft.com/dotnet/nightly/aspnet:8.0-preview +FROM mcr.microsoft.com/dotnet/aspnet:8.0-preview WORKDIR /app COPY --from=build-env /app/out . ENTRYPOINT ["dotnet", "InteropTestsWebsite.dll", "--port_http1", "80"] \ No newline at end of file From 97ffe5290ab8eaf75d40b7dc835f871d7dd7f06f Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Thu, 20 Apr 2023 22:20:40 +0800 Subject: [PATCH 3/4] Test STJ issue - add switch --- examples/Container/Backend/Dockerfile | 4 ++-- examples/Container/Frontend/Dockerfile | 4 ++-- .../InteropTestsGrpcWebClient.csproj | 3 ++- testassets/InteropTestsGrpcWebWebsite/Dockerfile | 4 ++-- testassets/InteropTestsWebsite/Dockerfile | 4 ++-- 5 files changed, 10 insertions(+), 9 deletions(-) diff --git a/examples/Container/Backend/Dockerfile b/examples/Container/Backend/Dockerfile index afa869b4e..19b794071 100644 --- a/examples/Container/Backend/Dockerfile +++ b/examples/Container/Backend/Dockerfile @@ -1,4 +1,4 @@ -FROM mcr.microsoft.com/dotnet/sdk:8.0-preview AS build-env +FROM mcr.microsoft.com/dotnet/nightly/sdk:8.0-preview AS build-env WORKDIR /app # Copy everything @@ -8,7 +8,7 @@ RUN dotnet restore examples/Container/Backend RUN dotnet publish examples/Container/Backend -c Release -o out # Build runtime image -FROM mcr.microsoft.com/dotnet/aspnet:8.0-preview +FROM mcr.microsoft.com/dotnet/nightly/aspnet:8.0-preview WORKDIR /app COPY --from=build-env /app/out . ENTRYPOINT ["dotnet", "Backend.dll"] \ No newline at end of file diff --git a/examples/Container/Frontend/Dockerfile b/examples/Container/Frontend/Dockerfile index 44634948c..09ebb01b3 100644 --- a/examples/Container/Frontend/Dockerfile +++ b/examples/Container/Frontend/Dockerfile @@ -1,4 +1,4 @@ -FROM mcr.microsoft.com/dotnet/sdk:8.0-preview AS build-env +FROM mcr.microsoft.com/dotnet/nightly/sdk:8.0-preview AS build-env WORKDIR /app # Copy everything @@ -8,7 +8,7 @@ RUN dotnet restore examples/Container/Frontend RUN dotnet publish examples/Container/Frontend -c Release -o out # Build runtime image -FROM mcr.microsoft.com/dotnet/aspnet:8.0-preview +FROM mcr.microsoft.com/dotnet/nightly/aspnet:8.0-preview WORKDIR /app COPY --from=build-env /app/out . ENTRYPOINT ["dotnet", "Frontend.dll"] \ No newline at end of file diff --git a/testassets/InteropTestsGrpcWebClient/InteropTestsGrpcWebClient.csproj b/testassets/InteropTestsGrpcWebClient/InteropTestsGrpcWebClient.csproj index 01e2690ed..56a80374c 100644 --- a/testassets/InteropTestsGrpcWebClient/InteropTestsGrpcWebClient.csproj +++ b/testassets/InteropTestsGrpcWebClient/InteropTestsGrpcWebClient.csproj @@ -1,8 +1,9 @@ - + net8.0 BLAZOR_WASM + true diff --git a/testassets/InteropTestsGrpcWebWebsite/Dockerfile b/testassets/InteropTestsGrpcWebWebsite/Dockerfile index 0115da9cb..cd82ea52b 100644 --- a/testassets/InteropTestsGrpcWebWebsite/Dockerfile +++ b/testassets/InteropTestsGrpcWebWebsite/Dockerfile @@ -1,4 +1,4 @@ -FROM mcr.microsoft.com/dotnet/sdk:8.0-preview AS build-env +FROM mcr.microsoft.com/dotnet/nightly/sdk:8.0-preview AS build-env WORKDIR /app # Copy everything @@ -8,7 +8,7 @@ RUN dotnet restore testassets/InteropTestsGrpcWebWebsite RUN dotnet publish testassets/InteropTestsGrpcWebWebsite -c Release -o out # Build runtime image -FROM mcr.microsoft.com/dotnet/aspnet:8.0-preview +FROM mcr.microsoft.com/dotnet/nightly/aspnet:8.0-preview WORKDIR /app COPY --from=build-env /app/out . ENTRYPOINT ["dotnet", "InteropTestsGrpcWebWebsite.dll", "--urls", "http://+:80"] diff --git a/testassets/InteropTestsWebsite/Dockerfile b/testassets/InteropTestsWebsite/Dockerfile index 81e4333b8..b60d6d6df 100644 --- a/testassets/InteropTestsWebsite/Dockerfile +++ b/testassets/InteropTestsWebsite/Dockerfile @@ -1,4 +1,4 @@ -FROM mcr.microsoft.com/dotnet/sdk:8.0-preview AS build-env +FROM mcr.microsoft.com/dotnet/nightly/sdk:8.0-preview AS build-env WORKDIR /app # Copy everything @@ -8,7 +8,7 @@ RUN dotnet restore testassets/InteropTestsWebsite RUN dotnet publish testassets/InteropTestsWebsite --framework net8.0 -c Release -o out -p:LatestFramework=true # Build runtime image -FROM mcr.microsoft.com/dotnet/aspnet:8.0-preview +FROM mcr.microsoft.com/dotnet/nightly/aspnet:8.0-preview WORKDIR /app COPY --from=build-env /app/out . ENTRYPOINT ["dotnet", "InteropTestsWebsite.dll", "--port_http1", "80"] \ No newline at end of file From 382deeb56086fa2769b19d09c174036f559ca94e Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Fri, 21 Apr 2023 07:07:02 +0800 Subject: [PATCH 4/4] Apply suggestions from code review Co-authored-by: Brennan --- .../Grpc.AspNetCore.Server.Tests/CallHandlerTests.cs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/test/Grpc.AspNetCore.Server.Tests/CallHandlerTests.cs b/test/Grpc.AspNetCore.Server.Tests/CallHandlerTests.cs index d4814d11d..369495fbb 100644 --- a/test/Grpc.AspNetCore.Server.Tests/CallHandlerTests.cs +++ b/test/Grpc.AspNetCore.Server.Tests/CallHandlerTests.cs @@ -191,11 +191,11 @@ public async Task RequestTimeoutFeature_Global_DisableWhenStreaming(MethodType m Assert.AreEqual(expectedTimeoutDisabled, timeoutFeature.TimeoutDisabled); } - [TestCase(MethodType.Unary, false)] - [TestCase(MethodType.ClientStreaming, false)] - [TestCase(MethodType.ServerStreaming, false)] - [TestCase(MethodType.DuplexStreaming, false)] - public async Task RequestTimeoutFeature_EndpointMetadata_DisableWhenStreaming(MethodType methodType, bool expectedTimeoutDisabled) + [TestCase(MethodType.Unary)] + [TestCase(MethodType.ClientStreaming)] + [TestCase(MethodType.ServerStreaming)] + [TestCase(MethodType.DuplexStreaming)] + public async Task RequestTimeoutFeature_WithEndpointMetadata_NotDisabledWhenStreaming(MethodType methodType) { // Arrange var timeoutFeature = new TestHttpRequestTimeoutFeature(); @@ -208,7 +208,7 @@ public async Task RequestTimeoutFeature_EndpointMetadata_DisableWhenStreaming(Me await call.HandleCallAsync(httpContext).DefaultTimeout(); // Assert - Assert.AreEqual(expectedTimeoutDisabled, timeoutFeature.TimeoutDisabled); + Assert.False(timeoutFeature.TimeoutDisabled); } private sealed class TestHttpRequestTimeoutFeature : IHttpRequestTimeoutFeature