From 07b8318db0ec2d9cf05a8ab2a24847342746a664 Mon Sep 17 00:00:00 2001 From: "Chris Ross (ASP.NET)" Date: Thu, 16 Jul 2020 21:37:46 +0000 Subject: [PATCH] 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/SendFileResponseExtensions.cs | 23 ++++- ...ft.AspNetCore.Http.Extensions.Tests.csproj | 4 + .../test/SendFileResponseExtensionsTests.cs | 95 ++++++++++++++++--- src/Http/Http.Extensions/test/testfile1kb.txt | 1 + .../StaticFiles/src/StaticFileContext.cs | 43 ++------- .../test/UnitTests/StaticFileContextTest.cs | 31 ++++++ 6 files changed, 144 insertions(+), 53 deletions(-) create mode 100644 src/Http/Http.Extensions/test/testfile1kb.txt diff --git a/src/Http/Http.Extensions/src/SendFileResponseExtensions.cs b/src/Http/Http.Extensions/src/SendFileResponseExtensions.cs index e7fd608c05..8dfae44bdc 100644 --- a/src/Http/Http.Extensions/src/SendFileResponseExtensions.cs +++ b/src/Http/Http.Extensions/src/SendFileResponseExtensions.cs @@ -16,6 +16,8 @@ namespace Microsoft.AspNetCore.Http /// public static class SendFileResponseExtensions { + private const int StreamCopyBufferSize = 64 * 1024; + /// /// Sends the given file using the SendFile extension. /// @@ -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(); - 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) diff --git a/src/Http/Http.Extensions/test/Microsoft.AspNetCore.Http.Extensions.Tests.csproj b/src/Http/Http.Extensions/test/Microsoft.AspNetCore.Http.Extensions.Tests.csproj index 19bf5e753a..fffeeb054b 100644 --- a/src/Http/Http.Extensions/test/Microsoft.AspNetCore.Http.Extensions.Tests.csproj +++ b/src/Http/Http.Extensions/test/Microsoft.AspNetCore.Http.Extensions.Tests.csproj @@ -4,6 +4,10 @@ $(DefaultNetCoreTargetFramework) + + + + diff --git a/src/Http/Http.Extensions/test/SendFileResponseExtensionsTests.cs b/src/Http/Http.Extensions/test/SendFileResponseExtensionsTests.cs index 134b882b99..f20675f9be 100644 --- a/src/Http/Http.Extensions/test/SendFileResponseExtensionsTests.cs +++ b/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(1, fakeFeature.offset); - Assert.Equal(3, fakeFeature.length); - Assert.Equal(CancellationToken.None, fakeFeature.token); + Assert.Equal("bob", fakeFeature.Name); + Assert.Equal(1, fakeFeature.Offset); + Assert.Equal(3, fakeFeature.Length); + 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( + () => 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(fakeFeature); + var response = context.Response; + + await Assert.ThrowsAsync( + () => 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(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 long offset = 0; - public long? length = null; - public CancellationToken token; + public string Name { get; set; } = null; + public long Offset { get; set; } = 0; + public long? Length { get; set; } = null; + 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; - this.offset = offset; - this.length = length; - this.token = cancellation; + Name = path; + Offset = offset; + Length = length; + Token = cancellation; + + cancellation.ThrowIfCancellationRequested(); return Task.FromResult(0); } diff --git a/src/Http/Http.Extensions/test/testfile1kb.txt b/src/Http/Http.Extensions/test/testfile1kb.txt new file mode 100644 index 0000000000..61d982da1a --- /dev/null +++ b/src/Http/Http.Extensions/test/testfile1kb.txt @@ -0,0 +1 @@ +aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa diff --git a/src/Middleware/StaticFiles/src/StaticFileContext.cs b/src/Middleware/StaticFiles/src/StaticFileContext.cs index 3de3576e76..f3864324bf 100644 --- a/src/Middleware/StaticFiles/src/StaticFileContext.cs +++ b/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(); - 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()) - { - // 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); - } + await _context.Response.SendFileAsync(_fileInfo, 0, _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(); } } @@ -390,31 +373,17 @@ namespace Microsoft.AspNetCore.StaticFiles _response.ContentLength = length; SetCompressionMode(); ApplyResponseHeaders(Constants.Status206PartialContent); - - string physicalPath = _fileInfo.PhysicalPath; - var sendFile = _context.Features.Get(); - 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()) - { - readStream.Seek(start, SeekOrigin.Begin); // TODO: What if !CanSeek? - _logger.CopyingFileRange(_response.Headers[HeaderNames.ContentRange], SubPath); - await StreamCopyOperation.CopyToAsync(readStream, _response.Body, length, _context.RequestAborted); - } + var logPath = !string.IsNullOrEmpty(_fileInfo.PhysicalPath) ? _fileInfo.PhysicalPath : SubPath; + _logger.SendingFileRange(_response.Headers[HeaderNames.ContentRange], logPath); + await _context.Response.SendFileAsync(_fileInfo, start, 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(); } } diff --git a/src/Middleware/StaticFiles/test/UnitTests/StaticFileContextTest.cs b/src/Middleware/StaticFiles/test/UnitTests/StaticFileContextTest.cs index efea2a08e5..409cda5b5b 100644 --- a/src/Middleware/StaticFiles/test/UnitTests/StaticFileContextTest.cs +++ b/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 _files = new Dictionary(StringComparer.Ordinal);