PR feedback

src/Grpc.AspNetCore.Web/Internal/GrpcWebFeature.cs

@@ -26,10 +26,15 @@
 
 namespace Grpc.AspNetCore.Web.Internal
 {
-    internal class GrpcWebFeature : IRequestBodyPipeFeature, IHttpResponseBodyFeature, IHttpResponseTrailersFeature
+    internal class GrpcWebFeature :
+        IRequestBodyPipeFeature,
+        IHttpResponseBodyFeature,
+        IHttpResponseTrailersFeature,
+        IHttpResetFeature
     {
         private readonly IHttpResponseBodyFeature _initialResponseFeature;
         private readonly IRequestBodyPipeFeature _initialRequestFeature;
+        private readonly IHttpResetFeature? _initialResetFeature;
 
         private readonly Base64PipeReader? _pipeReader;
         private readonly Base64PipeWriter? _pipeWriter;
@@ -40,6 +45,7 @@ public GrpcWebFeature(GrpcWebMode grpcWebMode, HttpContext httpContext)
         {
             _initialRequestFeature = httpContext.Features.Get<IRequestBodyPipeFeature>();
             _initialResponseFeature = httpContext.Features.Get<IHttpResponseBodyFeature>();
+            _initialResetFeature = httpContext.Features.Get<IHttpResetFeature>();
 
             if (grpcWebMode == GrpcWebMode.GrpcWebText)
             {
@@ -54,6 +60,8 @@ public GrpcWebFeature(GrpcWebMode grpcWebMode, HttpContext httpContext)
 
         public PipeWriter Writer => _pipeWriter ?? _initialResponseFeature.Writer;
 
+        // This could return a stream that does not base64 encode content.
+        // Shouldn't be a problem because gRPC only uses Response.BodyWriter.
         public Stream Stream => _initialResponseFeature.Stream;
 
         public IHeaderDictionary Trailers
@@ -68,20 +76,27 @@ public async Task CompleteAsync()
             // there is the potential for the main thread and CompleteAsync to both be writing to the response.
             // Shouldn't matter to the client because it will have already thrown deadline exceeded error, but
             // the response could return badly formatted trailers.
-            await WriteTrailers();
+            await WriteTrailersAsync();
             await _initialResponseFeature.CompleteAsync();
             _isComplete = true;
         }
 
         public void DisableBuffering() => _initialResponseFeature.DisableBuffering();
 
+        public void Reset(int errorCode)
+        {
+            // We set a reset feature so that HTTP/1.1 doesn't call HttpContext.Abort() on deadline.
+            // In HTTP/1.1 this will do nothing. In HTTP/2+ it will call the real reset feature.
+            _initialResetFeature?.Reset(errorCode);
+        }
+
         public Task SendFileAsync(string path, long offset, long? count, CancellationToken cancellationToken) =>
             _initialResponseFeature.SendFileAsync(path, offset, count, cancellationToken);
 
         public Task StartAsync(CancellationToken cancellationToken) =>
             _initialResponseFeature.StartAsync(cancellationToken);
 
-        public Task WriteTrailers()
+        public Task WriteTrailersAsync()
         {
             if (!_isComplete)
             {

src/Grpc.AspNetCore.Web/Internal/GrpcWebMiddleware.cs

@@ -85,7 +85,7 @@ private async Task HandleGrpcWebRequest(HttpContext httpContext, GrpcWebMode mod
 
             await _next(httpContext);
 
-            await feature.WriteTrailers();
+            await feature.WriteTrailersAsync();
         }
 
         private GrpcWebFeature SetGrpcWebFeature(GrpcWebMode mode, HttpContext httpContext)
@@ -95,6 +95,7 @@ private GrpcWebFeature SetGrpcWebFeature(GrpcWebMode mode, HttpContext httpConte
             httpContext.Features.Set<IRequestBodyPipeFeature>(grpcWebTextFeature);
             httpContext.Features.Set<IHttpResponseBodyFeature>(grpcWebTextFeature);
             httpContext.Features.Set<IHttpResponseTrailersFeature>(grpcWebTextFeature);
+            httpContext.Features.Set<IHttpResetFeature>(grpcWebTextFeature);
 
             return grpcWebTextFeature;
         }

src/Grpc.AspNetCore.Web/Internal/GrpcWebProtocolHelpers.cs

@@ -34,8 +34,8 @@ internal static class GrpcWebProtocolHelpers
         private const byte Space = (byte)' ';
 
         // Special trailers byte with eigth most significant bit set.
-        // Parsers will use this to identify regular messages vs trailers
-        private static readonly byte Trailers = 0x80;
+        // Parsers will use this to identify regular messages vs trailers.
+        private static readonly byte TrailersSignfier = 0x80;
 
         private static readonly int HeaderSize = 5;
 
@@ -65,7 +65,7 @@ internal static void WriteTrailers(IHeaderDictionary trailers, IBufferWriter<byt
 
         private static void WriteTrailersHeader(Span<byte> buffer, int length)
         {
-            buffer[0] = Trailers;
+            buffer[0] = TrailersSignfier;
             BinaryPrimitives.WriteUInt32BigEndian(buffer.Slice(1), (uint)length);
         }
 

test/FunctionalTests/Web/Server/DeadlineTests.cs

@@ -86,7 +86,8 @@ static async Task<HelloReply> WaitUntilDeadline(HelloRequest request, ServerCall
 
             var grpcWebClient = CreateGrpcWebClient();
 
-            for (int i = 0; i < 50; i++)
+            // TODO(JamesNK): This test is/was flaky. Remove loop if this test is no longer a problem
+            for (int i = 0; i < 20; i++)
             {
                 var requestMessage = new HelloRequest
                 {
@@ -100,19 +101,12 @@ static async Task<HelloReply> WaitUntilDeadline(HelloRequest request, ServerCall
                 httpRequest.Headers.Add(GrpcProtocolConstants.TimeoutHeader, "50m");
                 httpRequest.Content = new GrpcStreamContent(requestStream);
 
-                try
-                {
-                    // Act
-                    var response = await grpcWebClient.SendAsync(httpRequest, HttpCompletionOption.ResponseHeadersRead).DefaultTimeout();
+                // Act
+                var response = await grpcWebClient.SendAsync(httpRequest, HttpCompletionOption.ResponseHeadersRead).DefaultTimeout();
 
-                    // Assert
-                    response.AssertIsSuccessfulGrpcRequest();
-                    response.AssertTrailerStatus(StatusCode.DeadlineExceeded, "Deadline Exceeded");
-                }
-                catch (HttpRequestException ex) when (ex.InnerException?.Message == "The response ended prematurely.")
-                {
-                    // There seems to be some flakyness in HTTP/1 and CompleteAsync. Will sometimes never return data.
-                }
+                // Assert
+                response.AssertIsSuccessfulGrpcRequest();
+                response.AssertTrailerStatus(StatusCode.DeadlineExceeded, "Deadline Exceeded");
             }
         }
 

test/Grpc.AspNetCore.Server.Tests/Web/GrpcWebApplicationBuilderExtensionsTests.cs

@@ -17,7 +17,13 @@
 #endregion
 
 using System;
+using System.IO;
+using System.Threading.Tasks;
+using Grpc.AspNetCore.Web;
+using Grpc.AspNetCore.Web.Internal;
 using Microsoft.AspNetCore.Builder;
+using Microsoft.AspNetCore.Http;
+using Microsoft.AspNetCore.Http.Features;
 using Microsoft.Extensions.DependencyInjection;
 using NUnit.Framework;
 
@@ -50,5 +56,79 @@ public void UseGrpcWeb_WithServices_Success()
             // Act & Assert
             app.UseGrpcWeb();
         }
+
+        [Test]
+        public async Task UseGrpcWeb_CalledWithMatchingHttpContext_MiddlewareRuns()
+        {
+            // Arrange
+            var services = new ServiceCollection();
+            services.AddLogging();
+            services.AddGrpcWeb(o => o.GrpcWebEnabled = true);
+            var app = new ApplicationBuilder(services.BuildServiceProvider());
+
+            app.UseGrpcWeb();
+
+            var appFunc = app.Build();
+
+            var httpContext = new DefaultHttpContext();
+            httpContext.Request.Method = HttpMethods.Post;
+            httpContext.Request.ContentType = GrpcWebProtocolConstants.GrpcWebContentType;
+
+            // Act
+            await appFunc(httpContext);
+
+            // Assert
+            Assert.AreEqual(GrpcWebProtocolConstants.GrpcContentType, httpContext.Request.ContentType);
+        }
+
+        private class TestHttpResponseFeature : IHttpResponseFeature
+        {
+            public Stream Body { get; set; } = Stream.Null;
+            public bool HasStarted { get; }
+            public IHeaderDictionary Headers { get; set; } = new HeaderDictionary();
+            public string ReasonPhrase { get; set; } = string.Empty;
+            public int StatusCode { get; set; }
+
+            public void OnCompleted(Func<object, Task> callback, object state)
+            {
+                throw new NotImplementedException();
+            }
+
+            public void OnStarting(Func<object, Task> callback, object state)
+            {
+                StartingCallbackCount++;
+            }
+
+            public int StartingCallbackCount { get; private set; }
+        }
+
+        [Test]
+        public async Task UseGrpcWeb_RegisteredMultipleTimesCalledWithMatchingHttpContext_MiddlewareRuns()
+        {
+            // Arrange
+            var services = new ServiceCollection();
+            services.AddLogging();
+            services.AddGrpcWeb(o => o.GrpcWebEnabled = true);
+            var app = new ApplicationBuilder(services.BuildServiceProvider());
+
+            app.UseGrpcWeb();
+            app.UseGrpcWeb();
+
+            var appFunc = app.Build();
+
+            var httpContext = new DefaultHttpContext();
+            httpContext.Request.Method = HttpMethods.Post;
+            httpContext.Request.ContentType = GrpcWebProtocolConstants.GrpcWebContentType;
+
+            var testHttpResponseFeature = new TestHttpResponseFeature();
+            httpContext.Features.Set<IHttpResponseFeature>(testHttpResponseFeature);
+
+            // Act
+            await appFunc(httpContext);
+
+            // Assert
+            Assert.AreEqual(GrpcWebProtocolConstants.GrpcContentType, httpContext.Request.ContentType);
+            Assert.AreEqual(1, testHttpResponseFeature.StartingCallbackCount);
+        }
     }
 }