Merged PR 9144: [5.0-preview8] Pass RequestAborted to SendFileAsync

This is a cherry pick of the 3.1 PR.
https://dev.azure.com/dnceng/internal/_git/dotnet-aspnetcore/pullrequest/9042

The only change was to remove a questionable call to Abort after catching the exception.

There are some less severe issues in Kestrel and MVC that we'll follow up on publicly after this releases.

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,6 +1,7 @@
 // Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
+using System;
 using System.IO;
 using System.IO.Pipelines;
 using System.Threading;
@@ -29,18 +30,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();
 
@@ -58,10 +127,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,29 +341,14 @@ namespace Microsoft.AspNetCore.StaticFiles
         {
             SetCompressionMode();
             ApplyResponseHeaders(StatusCodes.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.
+                _logger.WriteCancelled(ex);
-                _context.Abort();
             }
         }
 
@@ -391,31 +372,16 @@ namespace Microsoft.AspNetCore.StaticFiles
             SetCompressionMode();
             ApplyResponseHeaders(StatusCodes.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.
+                _logger.WriteCancelled(ex);
-                _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);

src/Servers/HttpSys/test/FunctionalTests/ResponseSendFileTests.cs

@@ -487,17 +487,21 @@ namespace Microsoft.AspNetCore.Server.HttpSys
 
                 try
                 {
+                    // Note Response.SendFileAsync uses RequestAborted by default. This can cause the response to be disposed
+                    // before it throws an IOException, but there's a race depending on when the disconnect is noticed.
+                    // Passing our own token to skip that.
+                    using var cts = new CancellationTokenSource();
                     await Assert.ThrowsAsync<IOException>(async () =>
                     {
                         // It can take several tries before Send notices the disconnect.
                         for (int i = 0; i < Utilities.WriteRetryLimit; i++)
                         {
-                            await httpContext.Response.SendFileAsync(AbsoluteFilePath, 0, null);
+                            await httpContext.Response.SendFileAsync(AbsoluteFilePath, 0, null, cts.Token);
                         }
                     });
 
                     await Assert.ThrowsAsync<ObjectDisposedException>(() =>
-                        httpContext.Response.SendFileAsync(AbsoluteFilePath, 0, null));
+                        httpContext.Response.SendFileAsync(AbsoluteFilePath, 0, null, cts.Token));
 
                     testComplete.SetResult(0);
                 }
@@ -573,9 +577,13 @@ namespace Microsoft.AspNetCore.Server.HttpSys
             var testComplete = new TaskCompletionSource<int>(TaskCreationOptions.RunContinuationsAsynchronously);
             using (Utilities.CreateHttpServer(out var address, async httpContext =>
             {
+                // Note Response.SendFileAsync uses RequestAborted by default. This can cause the response to be disposed
+                // before it throws an IOException, but there's a race depending on when the disconnect is noticed.
+                // Passing our own token to skip that.
+                using var cts = new CancellationTokenSource();
                 httpContext.RequestAborted.Register(() => cancellationReceived.SetResult(0));
                 // First write sends headers
-                await httpContext.Response.SendFileAsync(AbsoluteFilePath, 0, null);
+                await httpContext.Response.SendFileAsync(AbsoluteFilePath, 0, null, cts.Token);
                 firstSendComplete.SetResult(0);
                 await clientDisconnected.Task;
 
@@ -586,7 +594,7 @@ namespace Microsoft.AspNetCore.Server.HttpSys
                         // It can take several tries before Write notices the disconnect.
                         for (int i = 0; i < Utilities.WriteRetryLimit; i++)
                         {
-                            await httpContext.Response.SendFileAsync(AbsoluteFilePath, 0, null, CancellationToken.None);
+                            await httpContext.Response.SendFileAsync(AbsoluteFilePath, 0, null, cts.Token);
                         }
                     });