Add range support #6150

jbagga · 2017-04-17T23:13:01Z

Addresses #3702

This PR adds range support for FileContentResult and FileStreamResult

WIP for PhysicalFileResult and VirtualFileResult

cc @pranavkm @Tratcher @javiercn I'd like some feedback on how to set this up. Right now there is a lot of back and forth between the base executor and the specific executors (mostly because the base executor has the code common for every file type).

javiercn

I'm not done, but here are a few things to improve

javiercn · 2017-04-18T00:49:19Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileContentResultExecutor.cs

+            var response = httpContext.Response;
+
+            var lastModified = result.LastModified;
+            var etag = result.EntityTag;


nit: I would just inline these variables given that you are already calling the method below within multiple lines.

javiercn · 2017-04-18T00:50:40Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileContentResultExecutor.cs

+            var etag = result.EntityTag;
+
+            var range = RangeHelper.ParseRange(
+                context: httpContext,


Avoid using named parameters here, they just add noise given that you have variables that explain what the parameter is for. I usually only use name parameters when passing literals, like null, true, "", 5, etc.

javiercn · 2017-04-18T00:52:20Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileContentResultExecutor.cs

        }

-        private static Task WriteFileAsync(ActionContext context, FileContentResult result)
+        private RangeItemHeaderValue SetContentRangeAndStatusCode(ActionContext context, FileContentResult result)


Seems like this method is duped and not trivial, can you figure out a way to share this logic?

jbagga · 2017-04-18T19:15:37Z

@javiercn Updated

Tratcher

What do you expect the end-to-end scenario to look like? Where should the If-Match and If-Modified-Since header be checked?
https://github.com/aspnet/StaticFiles/blob/dev/src/Microsoft.AspNetCore.StaticFiles/StaticFileContext.cs#L167-L219

Tratcher · 2017-04-18T19:23:15Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileContentResultExecutor.cs

+                return response.Body.WriteAsync(result.FileContents, offset: 0, count: result.FileContents.Length);
+            }
+
+            else if (range == null || rangeLength == 0)


if no range was specified (as opposed to a range length of 0) then you should send the whole body.

Tratcher · 2017-04-18T19:25:05Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileContentResultExecutor.cs

+                    return response.Body.WriteAsync(result.FileContents, offset: (int)range.From.Value, count: (int)rangeLength);
+                }
+
+                catch (OperationCanceledException ex)


Did you see this exception thrown? Kestrel and HttpSys don't throw this by default anymore, only if you pass in a CT.

I saw this in Static Files here

Ah, it's passing a CT, you're not.

Tratcher · 2017-04-18T19:26:54Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileResultExecutorBase.cs

+        protected long SetRangeHeaders(ActionContext context, FileResult result, RangeItemHeaderValue range)
+        {
+            var method = context.HttpContext.Request.Method;
+            var isGet = string.Equals("GET", method, StringComparison.OrdinalIgnoreCase);


https://github.com/aspnet/HttpAbstractions/blob/35cde79e46ec77c60f7546009ecf78c67b9b849a/src/Microsoft.AspNetCore.Http.Abstractions/HttpMethods.cs#L30

Tratcher · 2017-04-18T19:27:46Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileResultExecutorBase.cs

+        {
+            var method = context.HttpContext.Request.Method;
+            var isGet = string.Equals("GET", method, StringComparison.OrdinalIgnoreCase);
+            if (!isGet)


Tratcher · 2017-04-18T19:29:24Z

src/Microsoft.AspNetCore.Mvc.Core/FileStreamResult.cs

+        /// <summary>
+        /// Gets or sets the last modified information associated with the <see cref="FileStreamResult"/>.
+        /// </summary>
+        public DateTimeOffset LastModified { get; set; }


Nullable? This is optional

Okay. What about EntityTag? EntityTagHeaderValue is not nullable.

It's a class that can be null.

Tratcher · 2017-04-18T19:31:50Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileResultExecutorBase.cs

+            return range;
+        }
+
+        private ICollection<RangeItemHeaderValue> ParseRange(


This doesn't need to be a collection if you'll only ever return 0 or 1.

Tratcher · 2017-04-18T19:38:49Z

src/Microsoft.AspNetCore.Mvc.Core/FileStreamResult.cs

+            FileStream = fileStream;
+            if (enableRangeProcessing)
+            {
+                LastModified = lastModified;


These can be set regardless of range processing, they're just response headers

Tratcher · 2017-04-18T19:40:40Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileContentResultExecutor.cs

@@ -15,15 +17,54 @@ public FileContentResultExecutor(ILoggerFactory loggerFactory)

        public Task ExecuteAsync(ActionContext context, FileContentResult result)
        {
+            RangeItemHeaderValue range = new RangeItemHeaderValue(0, 0);


it's strange to initialize this, just leave it null

Tratcher · 2017-04-18T19:43:05Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileResultExecutorBase.cs

+            var response = context.HttpContext.Response;
+            var httpResponseHeaders = response.GetTypedHeaders();
+            bool rangeNotSatisfiable = false;
+            if (ranges == null || ranges.Count != 1)


This is overkill. It's not enough for ranges to be turned on, you must also do a soft-check for the right range headers and fall back to a normal response.
https://github.com/aspnet/StaticFiles/blob/dev/src/Microsoft.AspNetCore.StaticFiles/StaticFileMiddleware.cs#L103
https://github.com/aspnet/StaticFiles/blob/dev/src/Microsoft.AspNetCore.StaticFiles/StaticFileContext.cs#L221-L234

jbagga · 2017-04-19T00:31:11Z

Where should the If-Match and If-Modified-Since header be checked?

@Tratcher I didn't think these are specific to Range requests. Are they? They seem to be properties of the file.

Tratcher · 2017-04-19T17:12:21Z

I guess not, they have to do with the etag and last modified you're adding. An end-to-end would need to account for them somewhere.

jbagga · 2017-04-20T01:47:02Z

@Tratcher Updated. Working on the sample and figuring out the end to end experience.

javiercn · 2017-04-21T20:04:10Z

src/Microsoft.AspNetCore.Mvc.Core/FileContentResult.cs

+            MediaTypeHeaderValue contentType,
+            bool enableRangeProcessing,
+            DateTimeOffset? lastModified,
+            EntityTagHeaderValue entityTag = null)


We don't do default parameter values in public APIs in general (I believe)

javiercn · 2017-04-21T20:06:33Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileContentResultExecutor.cs

@@ -15,15 +17,53 @@ public FileContentResultExecutor(ILoggerFactory loggerFactory)

        public Task ExecuteAsync(ActionContext context, FileContentResult result)
        {
+            long rangeLength = default(long);


var rangeLength = 0L; 😃

javiercn · 2017-04-21T20:06:56Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileContentResultExecutor.cs

+                        result.LastModified.Value,
+                        result.EntityTag);
+                }
+


nit: remove white-space

javiercn · 2017-04-21T20:09:07Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileContentResultExecutor.cs

+            {
+                if (result.LastModified.HasValue)
+                {
+                    ComputeIfMatch(context, result, result.LastModified.Value, result.EntityTag);


remove result.LastModified.Value from there, it's not being used inside the method

javiercn · 2017-04-21T20:10:33Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileResultExecutorBase.cs

+            ActionContext context,
+            long fileLength,
+            DateTimeOffset? lastModified = null,
+            EntityTagHeaderValue etag = null)


No default parameter values :)

javiercn · 2017-04-21T20:12:19Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileResultExecutorBase.cs

+
+        private static PreconditionState GetMaxPreconditionState(params PreconditionState[] states)
+        {
+            PreconditionState max = PreconditionState.Unspecified;


javiercn · 2017-04-21T20:17:31Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileResultExecutorBase.cs

+            var range = ParseRange(context, fileLength, lastModified, etag);
+            var response = context.HttpContext.Response;
+            var httpResponseHeaders = response.GetTypedHeaders();
+            bool rangeNotSatisfiable = false;


rangeNotSatisfiable = range == null;

javiercn · 2017-04-21T20:18:07Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileResultExecutorBase.cs

+            httpResponseHeaders.LastModified = lastModified;
+            httpResponseHeaders.ETag = etag;
+
+            if (rangeNotSatisfiable)


You can just inline range == null here

javiercn · 2017-04-21T20:19:12Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileResultExecutorBase.cs

+                var normalizedRanges = RangeHelper.NormalizeRanges(range, fileLength);
+                return normalizedRanges.Single();
+            }
+


nit: extra line

javiercn · 2017-04-21T20:26:43Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileResultExecutorBase.cs

+            if (ifUnmodifiedSince.HasValue && ifUnmodifiedSince <= now)
+            {
+                bool unmodified = ifUnmodifiedSince >= lastModified;
+                _ifUnmodifiedSinceState = unmodified ? PreconditionState.ShouldProcess : PreconditionState.PreconditionFailed;


These classes are singletons, you can't add fields to them as one instance is shared across all requests of the app

javiercn · 2017-04-21T20:28:21Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileResultExecutorBase.cs

        }

        protected ILogger Logger { get; }

+        public PreconditionState GetPreconditionState()


I don't see this being used anywhere, so you can remove this, the public enum above and the GetMaxPreconditionState method below

kichalla · 2017-04-21T20:25:57Z

test/Microsoft.AspNetCore.Mvc.Core.Test/FileStreamResultTest.cs

+                entityTag: entityTag);
+
+            var httpContext = GetHttpContext();
+            httpContext.Request.GetTypedHeaders().Range = new RangeHeaderValue(start, end);


Can you add a test where the request does not have this range header and the action says enableRangeProcessing to true?

kichalla · 2017-04-21T20:27:07Z

test/Microsoft.AspNetCore.Mvc.Core.Test/FileStreamResultTest.cs

+            var actionContext = new ActionContext(httpContext, new RouteData(), new ActionDescriptor());
+
+            // Act
+            await result.ExecuteResultAsync(actionContext);


nit: move the following lines as part of Assert. It would be cleaner to see the part which does the main act

kichalla · 2017-04-21T20:33:22Z

test/Microsoft.AspNetCore.Mvc.Core.Test/FileStreamResultTest.cs

+
+        [Theory]
+        [InlineData(0, 4, "Hello", 5)]
+        [InlineData(6, 10, "World", 5)]


I recall there is a way to mention 'get me the content from 6th byte to the rest of the file' (without specifying the end index)...should we cover that scenario here or is it already covered in range helper tests?

https://tools.ietf.org/html/rfc7233#section-2.1 ..Look for the examples in these

Additional examples, assuming a representation of length 10000: o The final 500 bytes (byte offsets 9500-9999, inclusive): bytes=-500 Or: bytes=9500-

Ah, interesting. My view on this is that we should do nothing more than the Static Files middleware already does (not that I know what it supports in this case).

@Tratcher can answer if it is supported in StaticFiles but I believe it is. But I did add the support in this PR here: https://github.com/aspnet/Mvc/blob/jbagga/ByteRange3702/src/Microsoft.AspNetCore.Mvc.Core/Internal/FileResultExecutorBase.cs#L258-L265. Should I undo that?

jbagga · 2017-04-26T22:18:28Z

@javiercn @Tratcher @kichalla Updated

jbagga · 2017-04-26T23:03:17Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileResultExecutorBase.cs

+        }
+
+        // Internal for testing
+        internal static bool ComputeConditionalRequestHeaders(


@Tratcher Could you review this one in particular? I tried to follow this

kichalla · 2017-04-26T23:25:18Z

test/Microsoft.AspNetCore.Mvc.Core.Test/FileStreamResultTest.cs

+
+        [Theory]
+        [InlineData(0, 4, "Hello", 5)]
+        [InlineData(6, 10, "World", 5)]


https://tools.ietf.org/html/rfc7233#section-2.1 ..Look for the examples in these

Additional examples, assuming a representation of length 10000: o The final 500 bytes (byte offsets 9500-9999, inclusive): bytes=-500 Or: bytes=9500-

kichalla · 2017-04-26T23:26:10Z

test/Microsoft.AspNetCore.Mvc.Core.Test/FileStreamResultTest.cs

+            var actionContext = new ActionContext(httpContext, new RouteData(), new ActionDescriptor());
+
+            // Act
+            await result.ExecuteResultAsync(actionContext);


kichalla · 2017-04-26T23:29:49Z

test/Microsoft.AspNetCore.Mvc.FunctionalTests/FileResultTests.cs

+        [InlineData("0-6")]
+        [InlineData("bytes = 11-6")]
+        [InlineData("bytes = 1-4, 5-11")]
+        public async Task FileFromBinaryData_ReturnsFile_RangeRequestNotSatisfiable(string rangeString)


I could have missed it, but where is the test where a client gets AcceptRanges header in the response even though they did not make a range request? (Like this is how a client can know that ranges are supported)

I have this test. Is that what you mean? This is always set for all requests.

jbagga · 2017-04-27T01:33:52Z

@kichalla Updated

javiercn · 2017-04-26T22:36:42Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileContentResultExecutor.cs

-            return response.Body.WriteAsync(result.FileContents, offset: 0, count: result.FileContents.Length);
+            else
+            {
+                return response.Body.WriteAsync(result.FileContents, offset: (int)range.From.Value, count: (int)rangeLength);


This casting from long to int looks dangerous. Wouldn't this break in the case you are sending a file bigger than 4GB? @Tratcher

@jbagga Do we check anywhere that the range is within the limits of the file?

A client can limit the number of bytes requested without knowing the
size of the selected representation. If the last-byte-pos value is
absent, or if the value is greater than or equal to the current
length of the representation data, the byte range is interpreted as
the remainder of the representation (i.e., the server replaces the
value of last-byte-pos with a value that is one less than the current
length of the selected representation).

From this which we do here in the NormalizeRange method in the RangeHelper

This casting from long to int looks dangerous

I don't like it either. All the other file types use StreamCopyOperation here. @Tratcher any other way to do this?

@javiercn in this case the user has to have 4GB of content in memory which is a highly unlikely scenario

@kichalla Not at all. For 1, .NET on 64 bits, 2 go to Azure or AWS and see how many machines you can buy that have gazillions of GB of memory.

I have converted the byte array to a stream and used StreamCopyOperation like other FileResult types

javiercn · 2017-04-26T22:41:27Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileResultExecutorBase.cs

+        }
+
+        // Internal for testing
+        internal static bool ComputeConditionalRequestHeaders(


I believe you are not computing anything here, consider changing the method name to something like ShouldProcessConditionalRequestHeaders or something similar

javiercn · 2017-04-26T22:41:53Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileResultExecutorBase.cs

+            EntityTagHeaderValue etag = null)
+        {
+            // 14.24 If-Match
+            bool shouldProcess = false;


javiercn · 2017-04-26T22:44:21Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileResultExecutorBase.cs

+                    return false;
+                }
+            }
+


extra empty line

javiercn · 2017-04-26T22:44:49Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileResultExecutorBase.cs

+                var now = DateTimeOffset.UtcNow;
+                if (ifUnmodifiedSince.HasValue && lastModified.HasValue && ifUnmodifiedSince <= now)
+                {
+                    bool unmodified = ifUnmodifiedSince >= lastModified;


inline the variable

javiercn · 2017-04-27T17:07:55Z

src/Microsoft.AspNetCore.Mvc.Core/FileStreamResult.cs

+            Stream fileStream,
+            string contentType,
+            DateTimeOffset? lastModified,
+            EntityTagHeaderValue entityTag = null)


no default parameters

javiercn · 2017-04-27T17:17:18Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileResultExecutorBase.cs

+            var httpRequestHeaders = context.HttpContext.Request.GetTypedHeaders();
+            if (context.HttpContext.Request.Headers.ContainsKey(HeaderNames.Range))
+            {
+                bool shouldProcess = ComputeConditionalRequestHeaders(


This method ComputeConditionalRequestHeaders does too much, it not just only computes the headers but in some cases it also writes the response. I would suggest you factor this in a different way.
Have a method that tells you what to do, e.g. tells you if you should process the request headers or if you should send back precondition failed or other type of response.
Then based on the result do what you need to do (set the headers, send a precondition failed response, or similar).

javiercn · 2017-04-27T17:18:11Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileResultExecutorBase.cs

        public FileResultExecutorBase(ILogger logger)
        {
            Logger = logger;
        }

        protected ILogger Logger { get; }

-        protected void SetHeadersAndLog(ActionContext context, FileResult result)
+        protected (RangeItemHeaderValue range, long rangeLength)? SetHeadersAndLog(ActionContext context, FileResult result, long fileLength, DateTimeOffset? lastModified = null, EntityTagHeaderValue etag = null)
        {
            SetContentType(context, result);
            SetContentDispositionHeader(context, result);


I'm not sure if you should send these headers in the cases where you are sending precondition failed or a similar response @Tratcher

If precondition fails (412) or the response has not been modified (304), these are null and 0 respectively, which means the range will not be served, the entire file will be (which shouldn't happen so I am changing that right now). No other Range headers will be set either. @Tratcher do the Content-Range and Content-Length need to be set for these cases? It doesn't sound like it. https://tools.ietf.org/html/rfc7233#section-4.2

javiercn · 2017-04-27T17:21:40Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileStreamResultExecutor.cs

+                    {
+                        // 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.
+                        context.HttpContext.Abort();


Hmm, I'm not sure why are we adding this code, this seems like a different feature/bug

In the try block I am sending a CancellationToken to StreamCopyOperation.CopyToAsync. This is to catch the exception when the client disconnects.

javiercn · 2017-04-27T17:22:59Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/VirtualFileResultExecutor.cs

+                        fileStream.Seek(range.From.Value, SeekOrigin.Begin);
+                        await StreamCopyOperation.CopyToAsync(fileStream, response.Body, rangeLength, context.HttpContext.RequestAborted);
+                    }
+


nit: empty line

kichalla · 2017-04-27T18:14:37Z

src/Microsoft.AspNetCore.Mvc.Core/FileContentResult.cs

+        /// </summary>
+        /// <param name="fileContents">The bytes that represent the file contents.</param>
+        /// <param name="contentType">The Content-Type header of the response.</param>
+        /// <param name="lastModified">The <see cref="DateTimeOffset"/> of when the <see cref="FileContentResult"/>


nit: of when the <see cref="FileContentResult"/> this should reference the parameter 'fileContents'. You can use paramref

kichalla · 2017-04-27T18:15:00Z

src/Microsoft.AspNetCore.Mvc.Core/FileContentResult.cs

+        }
+
+        /// <summary>
+        /// Gets or sets the last modified information associated with the <see cref="FileStreamResult"/>.


with the <see cref="FileStreamResult"/> => should be FileContentResult

kichalla · 2017-04-27T18:15:14Z

src/Microsoft.AspNetCore.Mvc.Core/FileContentResult.cs

+        public DateTimeOffset? LastModified { get; set; }
+
+        /// <summary>
+        /// Gets or sets the etag associated with the <see cref="FileStreamResult"/>.


same here..update the comment

kichalla · 2017-04-27T18:18:15Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileContentResultExecutor.cs

-            return response.Body.WriteAsync(result.FileContents, offset: 0, count: result.FileContents.Length);
+            else
+            {
+                return response.Body.WriteAsync(result.FileContents, offset: (int)range.From.Value, count: (int)rangeLength);


@javiercn in this case the user has to have 4GB of content in memory which is a highly unlikely scenario

jbagga · 2017-04-28T01:15:55Z

@javiercn @kichalla @Tratcher Updated

Tratcher · 2017-04-28T03:00:32Z

src/Microsoft.AspNetCore.Mvc.Core/ControllerBase.cs

+        [NonAction]
+        public virtual FileStreamResult File(Stream fileStream, string contentType, string fileDownloadName, DateTimeOffset? lastModified, EntityTagHeaderValue entityTag)
+        {
+            return new FileStreamResult(fileStream, contentType, lastModified, entityTag) { FileDownloadName = fileDownloadName };


why are lastModified and entityTag constructor parameters but FileDownloadName is not? It looks like the stream and content-type are required, but these new ones are optional so you don't need the new constructors.

Tratcher · 2017-04-28T03:06:05Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileContentResultExecutor.cs

-            return WriteFileAsync(context, result);
+            RangeItemHeaderValue range;
+            long rangeLength;
+            if (result.LastModified.HasValue)


e-tags and last-modified can be processed independently, you should check if either of them are set.

Ah, you don't need this if-else, you can pass LastModified directly to SetHeadersAndLog because it accepts nullable.

.Value throws if .HasValue is false. So I added a check to set it to null if .HasValue is false

Tratcher · 2017-04-28T03:07:17Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileContentResultExecutor.cs

+            }
+
+            var statusCode = context.HttpContext.Response.StatusCode;
+            if (statusCode == StatusCodes.Status412PreconditionFailed ||


This check is redundant. SetHeadersAndLog can return a bool from the same code that sets the status codes.

Tratcher · 2017-04-28T03:10:43Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileResultExecutorBase.cs

+            return (null, 0);
+        }
+
+        private void RequestMethodHead()


Tratcher · 2017-04-28T03:16:07Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileStreamResultExecutor.cs

@@ -18,18 +23,72 @@ public FileStreamResultExecutor(ILoggerFactory loggerFactory)

        public Task ExecuteAsync(ActionContext context, FileStreamResult result)
        {
-            SetHeadersAndLog(context, result);
-            return WriteFileAsync(context, result);
+            var fileLength = 0L;


nullable? it won't be set for !CanSeek. It looks like this code sets Content-Length to 0 for non-seekable streams.

For non-seekable streams, calls to Length throw an exception. So I cannot get the length of the file. What's the expected behavior here? @Tratcher

If you can't get the length then you don't set content-length. Write the body normally and the server will set transfer-encoding: chunked.

You may not be able to calculate/verify ranges without the length,

Tratcher · 2017-04-28T03:17:10Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileResultExecutorBase.cs

+            if (context.HttpContext.Request.Headers.ContainsKey(HeaderNames.Range))
+            {
+                if (preconditionState.Equals(PreconditionState.Unspecified) ||
+                    preconditionState.Equals(PreconditionState.ShouldProcess))


skipped a set of { }

does this nested if mess up your next else if HEAD?

Tratcher · 2017-04-28T03:27:20Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/PhysicalFileResultExecutor.cs

        {
            var response = context.HttpContext.Response;
+            var fileStream = GetFileStream(result.FileName);


Why create the stream before the Path.IsPathRooted check?

Tratcher · 2017-04-28T03:29:37Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/PhysicalFileResultExecutor.cs

            {
-                var fileStream = GetFileStream(result.FileName);
-
                using (fileStream)
                {
                    await fileStream.CopyToAsync(response.Body, DefaultBufferSize);


Use StreamCopyOperation here too, you want the same cancellation logic if the writes start failing.

Tratcher · 2017-04-28T03:33:23Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/PhysicalFileResultExecutor.cs

            {
-                var fileStream = GetFileStream(result.FileName);
-
                using (fileStream)


GetFileStream and using (fileStream) need to stay together. Otherwise you miss closing the stream in a few places. You moved GetFileStream to avoid duplication for the last else block, but you end up opening the stream when you didn't need to for the (rangeLength == 0) scenario and forgetting to close it. Copy the GetFileStream call to your last else block and add a using statement there too. Even better: using (var fileStream = GetFileStream(result.FileName))

Tratcher · 2017-04-28T03:33:58Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/PhysicalFileResultExecutor.cs

+                    fileStream.Seek(range.From.Value, SeekOrigin.Begin);
+                    await StreamCopyOperation.CopyToAsync(fileStream, response.Body, rangeLength, context.HttpContext.RequestAborted);
+                }
+


empty line?

Tratcher · 2017-04-28T15:16:22Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/VirtualFileResultExecutor.cs

+        private async Task WriteFileAsync(ActionContext context, VirtualFileResult result, IFileInfo fileInfo, RangeItemHeaderValue range, long rangeLength)
+        {
+            var response = context.HttpContext.Response;
+            var fileStream = GetFileStream(fileInfo);


same problem, GetFileStream can't be separated from it's using statement.

Tratcher · 2017-04-28T15:17:08Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/VirtualFileResultExecutor.cs

                {
-                    var fileStream = GetFileStream(fileInfo);
                    using (fileStream)
                    {
                        await fileStream.CopyToAsync(response.Body, DefaultBufferSize);


StreamCopyOperation

Tratcher · 2017-04-28T15:23:13Z

src/Microsoft.AspNetCore.Mvc.Core/ControllerBase.cs

+            return new FileContentResult(fileContents, contentType, lastModified, entityTag) { FileDownloadName = fileDownloadName };
+        }
+
+        /// <summary>


What about etags for VirtualFileResult?

VirtualFileResult and PhysicalFileResult have FileInfo with LastModified date. They don't have etags. Should it be a property like in FileContentResult and FileStreamResult? @Tratcher

Could Etag and LastModified properties go on the base FileResult?

If it does, which LastModified will we look at? The one the user provides or the one from FileInfo? Or we only let users define the etag and not LastModified for Virtual and PhysicalFileResult? @Tratcher

Tratcher · 2017-04-28T15:25:15Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/VirtualFileResultExecutor.cs

@@ -59,14 +69,31 @@ public Task ExecuteAsync(ActionContext context, VirtualFileResult result)
                        count: null,
                        cancellation: default(CancellationToken));
                }
-                else
+                else if (range == null || !fileStream.CanSeek)


Even if you can't seek, you can still scan forward in the stream to get to a subrange.

How? If I set the count for a StreamCopyOperation to be the rangeLength but cannot set the beginning of the range to the range.From.Value, it wont be the correct range. @Tratcher

It may be moot, I don't think you can do your proper range verification without knowing the length.

I spoke to @Eilon and we decided that we will just not support ranges for CanSeek = false. So I treat it like the range was not given and return the entire body

Tratcher · 2017-04-28T15:28:14Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/VirtualFileResultExecutor.cs

+            var fileProvider = GetFileProvider(result);
+
+            var normalizedPath = result.FileName;
+            if (normalizedPath.StartsWith("~", StringComparison.Ordinal))


@sebastienros so ~ is meaningless for virtual files?

Tratcher · 2017-04-28T15:37:51Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/VirtualFileResultExecutor.cs

+            var fileInfo = GetFileInformation(result);
+            RangeItemHeaderValue range;
+            long rangeLength;
+            (range, rangeLength) = SetHeadersAndLog(


Need to confirm the file exists before setting headers. Move the FileNotFoundException all the way to the top?

jbagga · 2017-04-28T20:03:32Z

@Tratcher Addressed most of the feedback. Need clarifications for other points.

jbagga · 2017-04-28T21:40:32Z

@Tratcher Updated

Eilon · 2017-05-04T15:59:46Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileResultExecutorBase.cs

+            {
+                if (request.Headers.ContainsKey(HeaderNames.Range))
+                {
+                    if (preconditionState.Equals(PreconditionState.Unspecified) ||


Can just use == to compare enum values.

Eilon · 2017-05-04T16:00:40Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileResultExecutorBase.cs

+                }
+            }
+
+            return (null, 0, serveBody);


I don't recall with the new tuple syntax, but can you used the named values here? I.e. something like (range: null, rangeLength: 0, ...) ?

Eilon · 2017-05-04T16:01:48Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileResultExecutorBase.cs

+            return (null, 0, serveBody);
+        }
+
+        private void SetContentType(ActionContext context, FileResult result)


Could be made a static method? And the implementation can be just one line? context.HttpContext.Response.ContentType = result.ContentType;? And at that point, is this helper needed?

Not part of my change https://github.com/aspnet/Mvc/blob/dev/src/Microsoft.AspNetCore.Mvc.Core/Internal/FileResultExecutorBase.cs#L41. Should I still address the feedback?

Eilon · 2017-05-04T16:03:46Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileResultExecutorBase.cs

+            return max;
+        }
+
+        private (RangeItemHeaderValue range, long rangeLength) SetRangeHeaders(


Can some (or all?) of these utility methods be made static?

Eilon · 2017-05-04T16:04:28Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileResultExecutorBase.cs

+            if (range != null)
+            {
+                var rangeValue = range.Single();
+                long? from = rangeValue.From.HasValue ? rangeValue.From.Value : 0;


I think you can do rangeValue.From ?? 0 (the "null-coalescing operator").

Eilon · 2017-05-04T17:19:51Z

test/Microsoft.AspNetCore.Mvc.Core.Test/PhysicalFileResultTest.cs

+
+        private class TestSendFileFeature : IHttpSendFileFeature
+        {
+            public string name = null;


Don't set default values (they're the default!)

Maybe make these properties just for fun? Or at least change the case so it doesn't conflict with the parameters below.

Got this from here Should I change that as well?

Eilon · 2017-05-04T17:20:32Z

test/Microsoft.AspNetCore.Mvc.Core.Test/VirtualFileResultTest.cs

+
+        private class TestSendFileFeature : IHttpSendFileFeature
+        {
+            public string name = null;


See earlier comment; apply everywhere.

Eilon · 2017-05-04T17:21:33Z

test/Microsoft.AspNetCore.Mvc.FunctionalTests/FileResultTests.cs

@@ -37,6 +40,100 @@ public FileResultTests(MvcTestFixture<FilesWebSite.Startup> fixture)
            Assert.Equal("This is a sample text file", body);
        }

+        [ConditionalTheory]
+        // https://github.com/aspnet/Mvc/issues/2727
+        [FrameworkSkipCondition(RuntimeFrameworks.Mono)]


Doubt this condition is still needed??

Eilon · 2017-05-04T17:24:29Z

test/WebSites/FilesWebSite/Controllers/DownloadFilesController.cs

+        public IActionResult DownloadFromDisk_WithLastModifiedAndEtag()
+        {
+            var path = Path.Combine(_hostingEnvironment.ContentRootPath, "sample.txt");
+            var lastModified = DateTimeOffset.Parse("05/01/2008 +1:00");


DTO.Parse() uses the current culture, so this will not have the desired behavior on some machines. Either use ParseExact, or call a DTO ctor that takes in explicit year/month/day/time/etc. integers, e.g. https://msdn.microsoft.com/en-us/library/bb354978(v=vs.110).aspx

Eilon · 2017-05-04T17:24:38Z

test/WebSites/FilesWebSite/Controllers/DownloadFilesController.cs

+        public IActionResult DownloadFromDiskWithFileName_WithLastModifiedAndEtag()
+        {
+            var path = Path.Combine(_hostingEnvironment.ContentRootPath, "sample.txt");
+            var lastModified = DateTimeOffset.Parse("05/01/2008 +1:00");


See previous comment.

pranavkm · 2017-04-27T01:37:35Z

src/Microsoft.AspNetCore.Mvc.Core/FileStreamResult.cs

+            string contentType,
+            DateTimeOffset? lastModified,
+            EntityTagHeaderValue entityTag = null)
+            : base(contentType?.ToString())


Why the ToString()?

pranavkm · 2017-04-27T01:38:18Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileContentResultExecutor.cs

+            {
+                return response.Body.WriteAsync(result.FileContents, offset: 0, count: result.FileContents.Length);
+            }
+


pranavkm · 2017-04-27T01:39:52Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileResultExecutorBase.cs

+            var httpRequestHeaders = context.HttpContext.Request.GetTypedHeaders();
+            if (context.HttpContext.Request.Headers.ContainsKey(HeaderNames.Range))
+            {
+                bool shouldProcess = ComputeConditionalRequestHeaders(


var all day every day

pranavkm · 2017-04-28T21:46:00Z

src/Microsoft.AspNetCore.Mvc.Core/FileContentResult.cs

+        /// </summary>
+        public EntityTagHeaderValue EntityTag
+        {
+            get { return _entityTag ?? null; }


This seems odd

pranavkm · 2017-04-28T21:46:13Z

src/Microsoft.AspNetCore.Mvc.Core/FileContentResult.cs

+        ///// <param name="lastModified">The <see cref="DateTimeOffset"/> of when the <paramref name="fileContents"/>
+        ///// was last modified.</param>
+        ///// <param name="entityTag">The entity tag associated with the <paramref name="fileContents"/>.</param>
+        //public FileContentResult(


No commented code

pranavkm · 2017-05-04T18:10:53Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileContentResultExecutor.cs

        }

-        private static Task WriteFileAsync(ActionContext context, FileContentResult result)
+        private static async Task WriteFileAsync(ActionContext context, FileContentResult result, RangeItemHeaderValue range, long rangeLength)


Should be able to just return it (rather than awaiting the Task)

pranavkm · 2017-05-04T18:11:48Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileResultExecutorBase.cs

+        }
+
+        // Internal for testing
+        internal static PreconditionState CheckPreconditionHeaders(


GetPreconditionState \ CalculatePreconditionState?

pranavkm · 2017-05-04T18:13:36Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileResultExecutorBase.cs

+                        fileStream.Seek(range.From.Value, SeekOrigin.Begin);
+                        await StreamCopyOperation.CopyToAsync(fileStream, outputStream, rangeLength, BufferSize, context.RequestAborted);
+                    }
+


pranavkm · 2017-05-04T18:13:40Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileResultExecutorBase.cs

+                    {
+                        await StreamCopyOperation.CopyToAsync(fileStream, outputStream, null, BufferSize, context.RequestAborted);
+                    }
+


pranavkm · 2017-05-04T18:19:14Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/PhysicalFileResultExecutor.cs

+            };
+        }
+
+        protected class FileInfo


FileMetadata? PhysicalFileInfo is also a type from our libraries

jbagga · 2017-05-04T21:17:05Z

@Eilon @Tratcher Updated

Tratcher

Almost done. More questions and style than anything else.

Tratcher · 2017-05-09T20:39:41Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileResultExecutorBase.cs

        protected ILogger Logger { get; }

-        protected void SetHeadersAndLog(ActionContext context, FileResult result)
+        protected (RangeItemHeaderValue range, long rangeLength, bool serveBody) SetHeadersAndLog(ActionContext context, FileResult result, long? fileLength, DateTimeOffset? lastModified = null, EntityTagHeaderValue etag = null)


@Eilon are you OK with using tuples on public (or protected) APIs?

@Eilon? It's on the internal namespace so I think its OK, but want to get your input.

Formatting: This is impossible to read, format it as follows:
protected (RangeItemHeaderValue range, long rangeLength, bool serveBody) SetHeadersAndLog(
ActionContext context,
FileResult result,
....)
{
...
}

In general stick to less than 120 characters per line

Tratcher · 2017-05-09T20:41:06Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileContentResultExecutor.cs

        {
            var response = context.HttpContext.Response;
+            var outputStream = response.Body;
+            var fileContentsStream = new MemoryStream(result.FileContents);
+            if (range != null && rangeLength == 0)


why isn't this check first?

Tratcher · 2017-05-09T20:49:40Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileResultExecutorBase.cs

+                    preconditionState == PreconditionState.ShouldProcess)
+                {
+                    var rangeInfo = SetRangeHeaders(context, httpRequestHeaders, fileLength, lastModified, etag);
+                    if (response.StatusCode == StatusCodes.Status416RangeNotSatisfiable)


this should check a bool returned from SetRangeHeaders (e.g. serveBody?) rather than inspecting the response object for side-effects.

Tratcher · 2017-05-09T20:57:10Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileResultExecutorBase.cs

+        {
+            var response = context.HttpContext.Response;
+            var httpResponseHeaders = response.GetTypedHeaders();
+            var range = fileLength == null ? null : ParseRange(context, httpRequestHeaders, fileLength.Value, lastModified, etag);


fileLength.HasValue

Tratcher · 2017-05-09T20:59:47Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileResultExecutorBase.cs

+        {
+            var response = context.HttpContext.Response;
+            var httpResponseHeaders = response.GetTypedHeaders();
+            var range = fileLength == null ? null : ParseRange(context, httpRequestHeaders, fileLength.Value, lastModified, etag);


It's worth commenting that you explicitly checked for the presence of the range header prior to calling this method so you'd only end up with range == null for a parsing error (or the missing file length)

Tratcher · 2017-05-09T21:07:31Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileResultExecutorBase.cs

+
+            if (range != null)
+            {
+                var rangeValue = range.Single();


I'm wondering why ParseRange returned a collection when it repeatedly restricts it to a single range...

That's how the helper is set up. RangeHelper.ParseRange returns a collection of Ranges from the range header. Should we revisit the helper?

We can clean it up later.

Tratcher · 2017-05-09T21:14:45Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileResultExecutorBase.cs

+                    }
+                    else
+                    {
+                        fileStream.Seek(range.From.Value, SeekOrigin.Begin);


Have you confirmed this stream is seekable on all code paths? I only see one new check for CanSeek.

I didn't ask you to add another CanSeek check on 295, that's too late, you've already set headers. I asked if you had made sure that all streams that can be passed into WriteFileAsync are seekable. Check the call sites and see where those streams are coming from. E.g. MemoryStream and FileStream should be fine, but I don't think you have the same guarantee from VirtualFile?

For all file types except the FileStream, the stream is retrieved right before I call this method (in order to write to the response body). If I want to not set the headers for CanSeek = false streams, I should move the code to get the stream in ExecuteAsync before calling SetHeadersAndLog, check for CanSeek and if true send that in to the WriteFileAsync

Tratcher · 2017-05-09T21:26:54Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/PhysicalFileResultExecutor.cs

-            SetHeadersAndLog(context, result);
-            return WriteFileAsync(context, result);
+            var fileInfo = GetFileInfo(result.FileName);
+            if (fileInfo.Exists)


style: the nesting could be simplified by checking if (!fileInfo.Exists)

Tratcher · 2017-05-09T21:36:16Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/VirtualFileResultExecutor.cs

-
-            var fileInfo = fileProvider.GetFileInfo(normalizedPath);
-            if (fileInfo.Exists)
+            else


style: no else required after throwing

Tratcher · 2017-05-09T21:56:35Z

src/Microsoft.AspNetCore.Mvc.Core/ControllerBase.cs

+        }
+
+        /// <summary>
+        /// Returns a file with the specified range in the given <paramref name="fileContents" /> as content (<see cref="StatusCodes.Status206PartialContent"/> or


The ranges are not the point of the API (nor even a parameter to the API), they're an implementation detail. How about just putting something at the end of the summary that says "This supports range requests." and then we do a docs writeup for the details?

If lastModified and etag are not specified, the PreconditionState is Unspecified and the range may still be served if the Range header is correctly provided. So that additional comment for

"This supports range requests."

is relevant for all file controller actions in ControllerBase.

Also will it be useful to also mention the possible StatusCodes to expect (206 or 416) as part of the comment?

@Tratcher Example summary:

/// Returns a file in the specified <paramref name="fileStream" /> (<see cref="StatusCodes.Status200OK"/>), /// and the specified <paramref name="contentType" /> as the Content-Type. /// This supports range requests (<see cref="StatusCodes.Status206PartialContent"/> or /// <see cref="StatusCodes.Status416RangeNotSatisfiable"/> if the range is not satisfiable).

Better. I'll defer to @Eilon for MVC comment style and depth.

Yeah that looks great to me!

jbagga · 2017-05-09T23:59:48Z

@Tratcher @Eilon Updated

Tratcher · 2017-05-10T00:24:18Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileResultExecutorBase.cs

+                {
+                    httpResponseHeaders.ContentRange = new ContentRangeHeaderValue(fileLength.Value);
+                }
+                return (range: null, rangeLength: fileLength.Value, serveBody: false);


isn't range length 0 / ignored if range is null?

yes it is ignored essentially since this sets serveBody to false and returns from ExecuteAsync without writing to the response body

jbagga · 2017-05-10T18:54:02Z

@Eilon Could you read over the documentation in ControllerBase?

jbagga · 2017-05-11T19:28:56Z

@javiercn @kichalla @Tratcher @Eilon Updated. Please review or approve, thanks!

Tratcher · 2017-05-11T19:39:27Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/PhysicalFileResultExecutor.cs

+                        count: rangeLength,
+                        cancellation: default(CancellationToken));
+                }
+                else


style: flatten redundant else's?

javiercn · 2017-05-15T17:21:27Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileContentResultExecutor.cs

        }

-        private static Task WriteFileAsync(ActionContext context, FileContentResult result)
+        private Task WriteFileAsync(ActionContext context, FileContentResult result, RangeItemHeaderValue range, long rangeLength)


Why is this not static anymore?

javiercn · 2017-05-15T17:22:54Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileResultExecutorBase.cs

        protected ILogger Logger { get; }

-        protected void SetHeadersAndLog(ActionContext context, FileResult result)
+        protected (RangeItemHeaderValue range, long rangeLength, bool serveBody) SetHeadersAndLog(ActionContext context, FileResult result, long? fileLength, DateTimeOffset? lastModified = null, EntityTagHeaderValue etag = null)


@Eilon? It's on the internal namespace so I think its OK, but want to get your input.

javiercn · 2017-05-15T17:26:41Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileResultExecutorBase.cs

        protected ILogger Logger { get; }

-        protected void SetHeadersAndLog(ActionContext context, FileResult result)
+        protected (RangeItemHeaderValue range, long rangeLength, bool serveBody) SetHeadersAndLog(ActionContext context, FileResult result, long? fileLength, DateTimeOffset? lastModified = null, EntityTagHeaderValue etag = null)


Formatting: This is impossible to read, format it as follows:
protected (RangeItemHeaderValue range, long rangeLength, bool serveBody) SetHeadersAndLog(
ActionContext context,
FileResult result,
....)
{
...
}

In general stick to less than 120 characters per line

javiercn · 2017-05-15T17:32:18Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileResultExecutorBase.cs

+                serveBody = false;
+            }
+
+            if (request.Headers.ContainsKey(HeaderNames.Range))


request.Headers.ContainsKey(HeaderNames.Range) &&
(preconditionState == PreconditionState.Unspecified || preconditionState == PreconditionState.ShouldProcess))

javiercn · 2017-05-15T17:35:51Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileResultExecutorBase.cs

+            // Range may be null for parsing errors, multiple ranges and when the file length is missing.
+            var range = fileLength.HasValue ? ParseRange(context, httpRequestHeaders, fileLength.Value, lastModified, etag) : null;
+            var rangeNotSatisfiable = range == null;
+            if (rangeNotSatisfiable)


Inline this variable

javiercn · 2017-05-15T17:43:22Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileResultExecutorBase.cs

+
+            // 14.25 If-Modified-Since
+            var ifModifiedSince = httpRequestHeaders.IfModifiedSince;
+            if (ifModifiedSince.HasValue && ifModifiedSince <= now)


Why do you need ifModifiedSince <= now?

If ifModifiedSince > now, that's an invalid ifModifiedSince value

javiercn · 2017-05-15T17:47:58Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileResultExecutorBase.cs

+                ifUnmodifiedSinceState = unmodified ? PreconditionState.ShouldProcess : PreconditionState.PreconditionFailed;
+            }
+
+            var state = GetMaxPreconditionState(ifMatchState, ifNoneMatchState, ifModifiedSinceState, ifUnmodifiedSinceState);


I don't understand why this code gives preference to PreconditionFailed over NotModified. This computation can be done in place when processing each of the individual headers.

javiercn · 2017-05-15T18:06:43Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileResultExecutorBase.cs

+            }
+
+            var preconditionState = GetPreconditionState(context, httpRequestHeaders, lastModified, etag);
+            var serveBody = true;


This can be simplified to serveBody = !HttpMethods.IsHead(request.Method)

javiercn · 2017-05-15T18:18:46Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileResultExecutorBase.cs

+
+            // 14.24 If-Match
+            var ifMatch = httpRequestHeaders.IfMatch;
+            if (ifMatch != null && ifMatch.Any())


You can extract this block of code into a method and reuse it below, something like:
EtagMatches(ifMatch,etag,matchingResult,notMatchingResult){
if (ifMatch != null && ifMatch.Any())
{
ifMatchState = notMachingResult;
foreach (var entityTag in ifMatch)
{
if (entityTag.Equals(EntityTagHeaderValue.Any) || entityTag.Compare(etag, useStrongComparison: true))
{
ifMatchState = matchingResult;
break;
}
}
}
}

The same technique can be applied to the ifModifiedSince

For checking lastModified, the checks are trivial so I am not extracting out the method. I did for etag matching though

javiercn · 2017-05-15T21:15:54Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/FileResultExecutorBase.cs

+        }
+
+        // Internal for testing
+        internal static PreconditionState GetPreconditionState(


Should we do the checks in the following order? https://tools.ietf.org/html/rfc7232#section-6
@Tratcher

Hmm, that RFC was released about the time the original StaticFiles code was written...
Giving PreconditionFailed the highest priority has almost the same effect as that stated ordering.

jbagga · 2017-05-15T21:50:13Z

@javiercn @Eilon @kichalla Updated

jbagga · 2017-05-18T22:08:56Z

@javiercn Added If-Range header tests
cc @Eilon

dnfclas added the cla-already-signed label Apr 17, 2017

jbagga requested review from Tratcher and javiercn April 17, 2017 23:13

jbagga assigned javiercn Apr 17, 2017

jbagga force-pushed the jbagga/ByteRange3702 branch from b219722 to 9c55fd7 Compare April 17, 2017 23:16

jbagga requested a review from pranavkm April 17, 2017 23:46

javiercn suggested changes Apr 18, 2017

View reviewed changes

Tratcher reviewed Apr 18, 2017

View reviewed changes

jbagga force-pushed the jbagga/ByteRange3702 branch from 2b058f8 to e16ae56 Compare April 20, 2017 01:46

javiercn reviewed Apr 21, 2017

View reviewed changes

javiercn suggested changes Apr 21, 2017

View reviewed changes

kichalla suggested changes Apr 21, 2017

View reviewed changes

Tratcher mentioned this pull request Apr 24, 2017

Authentication in OnPrepareResponse aspnet/StaticFiles#181

Closed

jbagga force-pushed the jbagga/ByteRange3702 branch from e16ae56 to 926b169 Compare April 26, 2017 22:18

jbagga commented Apr 26, 2017

View reviewed changes

kichalla suggested changes Apr 26, 2017

View reviewed changes

javiercn suggested changes Apr 27, 2017

View reviewed changes

kichalla suggested changes Apr 27, 2017

View reviewed changes

Tratcher suggested changes Apr 28, 2017

View reviewed changes

Tratcher reviewed Apr 28, 2017

View reviewed changes

Eilon suggested changes May 4, 2017

View reviewed changes

pranavkm reviewed May 4, 2017

View reviewed changes

jbagga force-pushed the jbagga/ByteRange3702 branch from 9338778 to f7c5f68 Compare May 4, 2017 21:15

jbagga assigned Tratcher May 9, 2017

Tratcher reviewed May 9, 2017

View reviewed changes

jbagga force-pushed the jbagga/ByteRange3702 branch from f7c5f68 to 7edff12 Compare May 9, 2017 23:58

Tratcher reviewed May 10, 2017

View reviewed changes

Tratcher removed their assignment May 10, 2017

jbagga force-pushed the jbagga/ByteRange3702 branch from e915231 to ead103e Compare May 11, 2017 19:27

Tratcher approved these changes May 11, 2017

View reviewed changes

Tratcher mentioned this pull request May 12, 2017

Add StaticFileResult #6260

Closed

javiercn suggested changes May 15, 2017

View reviewed changes

javiercn reviewed May 15, 2017

View reviewed changes

jbagga force-pushed the jbagga/ByteRange3702 branch from 8415a44 to 249b6a4 Compare May 15, 2017 21:49

jbagga force-pushed the jbagga/ByteRange3702 branch from 249b6a4 to 09b894e Compare May 16, 2017 23:45

Add Range support

d1a0b42

jbagga force-pushed the jbagga/ByteRange3702 branch from c863a85 to d1a0b42 Compare May 18, 2017 22:02

Add If-Range check

88dd351

javiercn approved these changes May 19, 2017

View reviewed changes

Add IgnoreRangeRequest unit test

0b5d257

jbagga merged commit 9aff0a6 into dev May 19, 2017

jbagga deleted the jbagga/ByteRange3702 branch May 19, 2017 17:51

jbagga mentioned this pull request May 25, 2017

FileStreamResult very slow #6045

Closed

guardrex mentioned this pull request Mar 12, 2018

2.0 Range requests in MVC dotnet/AspNetCore.Docs#3657

Closed

Add range support #6150

Add range support #6150

Conversation

jbagga commented Apr 17, 2017

javiercn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbagga commented Apr 18, 2017

Tratcher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbagga commented Apr 19, 2017 • edited Loading

Tratcher commented Apr 19, 2017

jbagga commented Apr 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbagga May 4, 2017 • edited Loading

Choose a reason for hiding this comment

jbagga commented Apr 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbagga commented Apr 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbagga Apr 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbagga Apr 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbagga commented Apr 28, 2017

Choose a reason for hiding this comment

jbagga commented Apr 19, 2017 •

edited

Loading

jbagga May 4, 2017 •

edited

Loading

jbagga Apr 27, 2017 •

edited

Loading

jbagga Apr 27, 2017 •

edited

Loading