Merged PR 9042: [3.1] Pass RequestAborted to SendFileAsync

This is the 3.1 version of https://dev.azure.com/dnceng/internal/_git/dotnet-aspnetcore/pullrequest/9014.

The concerns in 3.1 are slightly different than prior versions due to a significant redesign in how the response body was handled.

In 2.1 very few components implemented IHttpSendFileFeature, and most of those that did would eagerly terminate if the request aborted (e.g. HttpSys server). We mainly had to be concerned about the fallback code that did a copy loop when IHttpSendFileFeature wasn't available.

In 3.x the response body Stream, PipeWriter, and SendFileAsync were consolidated onto the new IHttpResponseBodyFeature. Now all servers and component shims support all three ways to send data and we can't make any assumptions about how eagerly they terminate. E.g. many components implemented SendFileAsync using a fallback copy loop, and these components don't have access to RequestAborted to eagerly terminate. This means that in 3.1 we need to pass the RequestAborted token when calling IHttpSendFileFeature.SendFileAsync, as well as any copy loops that have access to the token.

I've primarily fixed the HttpResponse.SendFileAsync extension methods and made sure the other affected components call through here.

[Infrastructure side note]
This commit needs to be rebased on internal/release/3.1 before merging. That branch can't be built locally so I developed this fix based on release/3.1 instead.

src/Http/Http.Extensions/src/SendFileResponseExtensions.cs

@@ -16,6 +16,8 @@ namespace Microsoft.AspNetCore.Http
     /// </summary>
     public static class SendFileResponseExtensions
     {
+        private const int StreamCopyBufferSize = 64 * 1024;
+
         /// <summary>
         /// Sends the given file using the SendFile extension.
         /// </summary>
@@ -110,15 +112,21 @@ namespace Microsoft.AspNetCore.Http
             if (string.IsNullOrEmpty(file.PhysicalPath))
             {
                 CheckRange(offset, count, file.Length);
+                using var fileContent = file.CreateReadStream();
 
-                using (var fileContent = file.CreateReadStream())
+                var useRequestAborted = !cancellationToken.CanBeCanceled;
+                var localCancel = useRequestAborted ? response.HttpContext.RequestAborted : cancellationToken;
+
+                try
                 {
+                    localCancel.ThrowIfCancellationRequested();
                     if (offset > 0)
                     {
                         fileContent.Seek(offset, SeekOrigin.Begin);
                     }
-                    await StreamCopyOperation.CopyToAsync(fileContent, response.Body, count, cancellationToken);
+                    await StreamCopyOperation.CopyToAsync(fileContent, response.Body, count, StreamCopyBufferSize, localCancel);
                 }
+                catch (OperationCanceledException) when (useRequestAborted) { }
             }
             else
             {
@@ -126,10 +134,17 @@ namespace Microsoft.AspNetCore.Http
             }
         }
 
-        private static Task SendFileAsyncCore(HttpResponse response, string fileName, long offset, long? count, CancellationToken cancellationToken = default)
+        private static async Task SendFileAsyncCore(HttpResponse response, string fileName, long offset, long? count, CancellationToken cancellationToken = default)
         {
+            var useRequestAborted = !cancellationToken.CanBeCanceled;
+            var localCancel = useRequestAborted ? response.HttpContext.RequestAborted : cancellationToken;
             var sendFile = response.HttpContext.Features.Get<IHttpResponseBodyFeature>();
-            return sendFile.SendFileAsync(fileName, offset, count, cancellationToken);
+
+            try
+            {
+                await sendFile.SendFileAsync(fileName, offset, count, localCancel);
+            }
+            catch (OperationCanceledException) when (useRequestAborted) { }
         }
 
         private static void CheckRange(long offset, long? count, long fileLength)

src/Http/Http.Extensions/test/Microsoft.AspNetCore.Http.Extensions.Tests.csproj

@@ -4,6 +4,10 @@
     <TargetFramework>$(DefaultNetCoreTargetFramework)</TargetFramework>
   </PropertyGroup>
 
+  <ItemGroup>
+    <Content Include="testfile1kb.txt" CopyToOutputDirectory="PreserveNewest" />
+  </ItemGroup>
+
   <ItemGroup>
     <Reference Include="Microsoft.AspNetCore.Http" />
     <Reference Include="Microsoft.AspNetCore.Http.Extensions" />

src/Http/Http.Extensions/test/SendFileResponseExtensionsTests.cs

@@ -1,5 +1,6 @@
 // Copyright (c) .NET Foundation. All rights reserved. See License.txt in the project root for license information.
 
+using System;
 using System.IO;
 using System.IO.Pipelines;
 using System.Threading;
@@ -28,18 +29,86 @@ namespace Microsoft.AspNetCore.Http.Extensions.Tests
 
             await response.SendFileAsync("bob", 1, 3, CancellationToken.None);
 
-            Assert.Equal("bob", fakeFeature.name);
+            Assert.Equal("bob", fakeFeature.Name);
-            Assert.Equal(1, fakeFeature.offset);
+            Assert.Equal(1, fakeFeature.Offset);
-            Assert.Equal(3, fakeFeature.length);
+            Assert.Equal(3, fakeFeature.Length);
-            Assert.Equal(CancellationToken.None, fakeFeature.token);
+            Assert.Equal(CancellationToken.None, fakeFeature.Token);
+        }
+
+        [Fact]
+        public async Task SendFile_FallsBackToBodyStream()
+        {
+            var body = new MemoryStream();
+            var context = new DefaultHttpContext();
+            var response = context.Response;
+            response.Body = body;
+
+            await response.SendFileAsync("testfile1kb.txt", 1, 3, CancellationToken.None);
+
+            Assert.Equal(3, body.Length);
+        }
+
+        [Fact]
+        public async Task SendFile_Stream_ThrowsWhenCanceled()
+        {
+            var body = new MemoryStream();
+            var context = new DefaultHttpContext();
+            var response = context.Response;
+            response.Body = body;
+
+            await Assert.ThrowsAnyAsync<OperationCanceledException>(
+                () => response.SendFileAsync("testfile1kb.txt", 1, 3, new CancellationToken(canceled: true)));
+
+            Assert.Equal(0, body.Length);
+        }
+
+        [Fact]
+        public async Task SendFile_Feature_ThrowsWhenCanceled()
+        {
+            var context = new DefaultHttpContext();
+            var fakeFeature = new FakeResponseBodyFeature();
+            context.Features.Set<IHttpResponseBodyFeature>(fakeFeature);
+            var response = context.Response;
+
+            await Assert.ThrowsAsync<OperationCanceledException>(
+                () => response.SendFileAsync("testfile1kb.txt", 1, 3, new CancellationToken(canceled: true)));
+        }
+
+        [Fact]
+        public async Task SendFile_Stream_AbortsSilentlyWhenRequestCanceled()
+        {
+            var body = new MemoryStream();
+            var context = new DefaultHttpContext();
+            context.RequestAborted = new CancellationToken(canceled: true);
+            var response = context.Response;
+            response.Body = body;
+
+            await response.SendFileAsync("testfile1kb.txt", 1, 3, CancellationToken.None);
+
+            Assert.Equal(0, body.Length);
+        }
+
+        [Fact]
+        public async Task SendFile_Feature_AbortsSilentlyWhenRequestCanceled()
+        {
+            var context = new DefaultHttpContext();
+            var fakeFeature = new FakeResponseBodyFeature();
+            context.Features.Set<IHttpResponseBodyFeature>(fakeFeature);
+            var token = new CancellationToken(canceled: true);
+            context.RequestAborted = token;
+            var response = context.Response;
+
+            await response.SendFileAsync("testfile1kb.txt", 1, 3, CancellationToken.None);
+
+            Assert.Equal(token, fakeFeature.Token);
         }
 
         private class FakeResponseBodyFeature : IHttpResponseBodyFeature
         {
-            public string name = null;
+            public string Name { get; set; } = null;
-            public long offset = 0;
+            public long Offset { get; set; } = 0;
-            public long? length = null;
+            public long? Length { get; set; } = null;
-            public CancellationToken token;
+            public CancellationToken Token { get; set; }
 
             public Stream Stream => throw new System.NotImplementedException();
 
@@ -57,10 +126,12 @@ namespace Microsoft.AspNetCore.Http.Extensions.Tests
 
             public Task SendFileAsync(string path, long offset, long? length, CancellationToken cancellation)
             {
-                this.name = path;
+                Name = path;
-                this.offset = offset;
+                Offset = offset;
-                this.length = length;
+                Length = length;
-                this.token = cancellation;
+                Token = cancellation;
+
+                cancellation.ThrowIfCancellationRequested();
                 return Task.FromResult(0);
             }
 

src/Http/Http.Extensions/test/testfile1kb.txt

@@ -0,0 +1 @@
+aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa

src/Middleware/StaticFiles/src/StaticFileContext.cs

@@ -5,11 +5,9 @@ using System;
 using System.Diagnostics;
 using System.IO;
 using System.Linq;
-using System.Threading;
 using System.Threading.Tasks;
 using Microsoft.AspNetCore.Builder;
 using Microsoft.AspNetCore.Http;
-using Microsoft.AspNetCore.Http.Extensions;
 using Microsoft.AspNetCore.Http.Features;
 using Microsoft.AspNetCore.Http.Headers;
 using Microsoft.AspNetCore.Internal;
@@ -21,8 +19,6 @@ namespace Microsoft.AspNetCore.StaticFiles
 {
     internal struct StaticFileContext
     {
-        private const int StreamCopyBufferSize = 64 * 1024;
-
         private readonly HttpContext _context;
         private readonly StaticFileOptions _options;
         private readonly HttpRequest _request;
@@ -345,28 +341,15 @@ namespace Microsoft.AspNetCore.StaticFiles
         {
             SetCompressionMode();
             ApplyResponseHeaders(Constants.Status200Ok);
-            string physicalPath = _fileInfo.PhysicalPath;
-            var sendFile = _context.Features.Get<IHttpResponseBodyFeature>();
-            if (sendFile != null && !string.IsNullOrEmpty(physicalPath))
-            {
-                // We don't need to directly cancel this, if the client disconnects it will fail silently.
-                await sendFile.SendFileAsync(physicalPath, 0, _length, CancellationToken.None);
-                return;
-            }
-
             try
             {
-                using (var readStream = _fileInfo.CreateReadStream())
+                await _context.Response.SendFileAsync(_fileInfo, 0, _length, _context.RequestAborted);
-                {
-                    // Larger StreamCopyBufferSize is required because in case of FileStream readStream isn't going to be buffering
-                    await StreamCopyOperation.CopyToAsync(readStream, _response.Body, _length, StreamCopyBufferSize, _context.RequestAborted);
-                }
             }
             catch (OperationCanceledException ex)
             {
                 _logger.WriteCancelled(ex);
                 // Don't throw this exception, it's most likely caused by the client disconnecting.
-                // However, if it was cancelled for any other reason we need to prevent empty responses.
+                // However, if it was canceled for any other reason we need to prevent empty responses.
                 _context.Abort();
             }
         }
@@ -390,31 +373,17 @@ namespace Microsoft.AspNetCore.StaticFiles
             _response.ContentLength = length;
             SetCompressionMode();
             ApplyResponseHeaders(Constants.Status206PartialContent);
-
-            string physicalPath = _fileInfo.PhysicalPath;
-            var sendFile = _context.Features.Get<IHttpResponseBodyFeature>();
-            if (sendFile != null && !string.IsNullOrEmpty(physicalPath))
-            {
-                _logger.SendingFileRange(_response.Headers[HeaderNames.ContentRange], physicalPath);
-                // We don't need to directly cancel this, if the client disconnects it will fail silently.
-                await sendFile.SendFileAsync(physicalPath, start, length, CancellationToken.None);
-                return;
-            }
-
             try
             {
-                using (var readStream = _fileInfo.CreateReadStream())
+                var logPath = !string.IsNullOrEmpty(_fileInfo.PhysicalPath) ? _fileInfo.PhysicalPath : SubPath;
-                {
+                _logger.SendingFileRange(_response.Headers[HeaderNames.ContentRange], logPath);
-                    readStream.Seek(start, SeekOrigin.Begin); // TODO: What if !CanSeek?
+                await _context.Response.SendFileAsync(_fileInfo, start, length, _context.RequestAborted);
-                    _logger.CopyingFileRange(_response.Headers[HeaderNames.ContentRange], SubPath);
-                    await StreamCopyOperation.CopyToAsync(readStream, _response.Body, length, _context.RequestAborted);
-                }
             }
             catch (OperationCanceledException ex)
             {
                 _logger.WriteCancelled(ex);
                 // Don't throw this exception, it's most likely caused by the client disconnecting.
-                // However, if it was cancelled for any other reason we need to prevent empty responses.
+                // However, if it was canceled for any other reason we need to prevent empty responses.
                 _context.Abort();
             }
         }

src/Middleware/StaticFiles/test/UnitTests/StaticFileContextTest.cs

@@ -4,6 +4,7 @@
 using System;
 using System.Collections.Generic;
 using System.IO;
+using System.Threading;
 using System.Threading.Tasks;
 using Microsoft.AspNetCore.Builder;
 using Microsoft.AspNetCore.Http;
@@ -120,6 +121,36 @@ namespace Microsoft.AspNetCore.StaticFiles
             Assert.Equal(HttpsCompressionMode.Default, httpsCompressionFeature.Mode);
         }
 
+        [Fact]
+        public async Task RequestAborted_DoesntThrow()
+        {
+            var options = new StaticFileOptions();
+            var fileProvider = new TestFileProvider();
+            fileProvider.AddFile("/foo.txt", new TestFileInfo
+            {
+                LastModified = new DateTimeOffset(2014, 1, 2, 3, 4, 5, TimeSpan.Zero)
+            });
+            var pathString = new PathString("/test");
+            var httpContext = new DefaultHttpContext();
+            httpContext.Request.Path = new PathString("/test/foo.txt");
+            httpContext.RequestAborted = new CancellationToken(canceled: true);
+            var body = new MemoryStream();
+            httpContext.Response.Body = body;
+            var validateResult = StaticFileMiddleware.ValidatePath(httpContext, pathString, out var subPath);
+            var contentTypeResult = StaticFileMiddleware.LookupContentType(new FileExtensionContentTypeProvider(), options, subPath, out var contentType);
+
+            var context = new StaticFileContext(httpContext, options, NullLogger.Instance, fileProvider, contentType, subPath);
+
+            var result = context.LookupFileInfo();
+            Assert.True(validateResult);
+            Assert.True(contentTypeResult);
+            Assert.True(result);
+
+            await context.SendAsync();
+
+            Assert.Equal(0, body.Length);
+        }
+
         private sealed class TestFileProvider : IFileProvider
         {
             private readonly Dictionary<string, IFileInfo> _files = new Dictionary<string, IFileInfo>(StringComparer.Ordinal);