Skip to content
This repository has been archived by the owner on Dec 14, 2018. It is now read-only.

Add EnableRangeProcessing #6895

Merged
merged 1 commit into from
Sep 30, 2017
Merged

Add EnableRangeProcessing #6895

merged 1 commit into from
Sep 30, 2017

Conversation

jbagga
Copy link
Contributor

@jbagga jbagga commented Sep 28, 2017

Addresses #6780

Ensured that this change solves the multiple GET requests issue described in #6780.

@Tratcher @rynowak

@@ -41,6 +41,7 @@ internal enum PreconditionState
ActionContext context,
FileResult result,
long? fileLength,
bool enableRangeProcessing,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is .Infrastructure considered public API? the class is public and the method is protected, so people could derive from it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was .Internal until recently.

@Eilon is Infrastructure just a name change and is considered internal still?

Copy link
Member

@rynowak rynowak Sep 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jbagga was this in .Infrastructure in 2.0.0? That's the salient point

If this class was in .Internal in 2.0.0 then it has no support bar in 2.0.0 and there is no breaking change 😆

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or rather, it's a change that we're willing to make

Copy link
Contributor Author

@jbagga jbagga Sep 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jbagga jbagga requested a review from Eilon September 30, 2017 01:05
@jbagga
Copy link
Contributor Author

jbagga commented Sep 30, 2017

@Eilon since .Infrastructure is a public namespace, this is a breaking change

Edit: This was .Internal in 2.0.0, so it isn't a breaking change. Merging it in

@jbagga jbagga merged commit 2fcfc6b into dev Sep 30, 2017
@jbagga jbagga deleted the jbagga/EnableRangeProcessing6780 branch September 30, 2017 01:39
@spallister
Copy link

@jbagga - Could you please confirm if range processing, to enable 206 Partial Content responses, will not be in ASP.NET Core until 2.1, which has yet to be released? Reason I'm asking is because the online documentation here for ASP.NET Core 2.0 - which I'm currently using - seems to imply that this is already implemented:

Returns the file specified by virtualPath (Status200OK) with the specified contentType as the Content-Type. This supports range requests (Status206PartialContent or Status416RangeNotSatisfiable if the range is not satisfiable).

Intellisense within Visual Studio is similarly mentioning support for range requests when working with File Result Executors (e.g. ControllerBase.File), however, the action always returns 200 OK results, and correspondingly, the Content-Range header is never returned in the response.

Thanks in advance for your help!

@jbagga
Copy link
Contributor Author

jbagga commented Jan 9, 2018

@spallister It should work for 2.0.0 by default. Can you share a repro and the exact SDK you are using? In 2.0.x patch we added a switch to turn on range processing #6792 (not ON by default).

@spallister
Copy link

@jbagga - I was using ASP.NET Core 2.0.3, then just tried updating to ASP.NET Core 2.0.5, and see that I'm using 2.1.4 of the .NET Core SDK upon running dotnet --version in the Package Manager Console of Visual Studio - same result.

Here's the action that I'm trying to execute:

/// <summary>
/// Get Video File by ID.
/// </summary>
[HttpGet("{id}/file")]
public async Task<FileStreamResult> GetVideoFileById(Guid id)
{
    var query = new GetVideoFilePathQuery(id);

    var videoFilePathDto = await _mediator.Send(query);

    if (videoFilePathDto != null)
    {
        return File(System.IO.File.OpenRead(videoFilePathDto.FilePath), videoFilePathDto.FileContentType);
    }
    else
    {
        return null;
    }
}

I was able to successfully return 206 Partial Content responses using a SO contributor's VideoStreamResult implementation here (e.g. replaced FileStreamResult and File with VideoStreamResult and new VideoStreamResult correspondingly, in the above).

For the switch you mention in your above post, how do I go about enabling it (i.e. where is it located)?

@jbagga
Copy link
Contributor Author

jbagga commented Jan 10, 2018

You can set the switch like this example in a runtimeconfig.json file for .NET Core

{
  // Set the switch here to affect .NET Core apps
  "configProperties": {
    "Switch.Microsoft.AspNetCore.Mvc.EnableRangeProcessing": "true"
  }
}

This will turn on range processing for FileStreamResult and FileContentResult for 2.0.x. If this does not solve your problem, please open an issue with a repro and I will investigate further!

Edit: For dev and 2.1.x, using an overload of File with bool enableRangeProcessing set to true will suffice. No need for a switch

@spallister
Copy link

That's got it - Thanks, @jbagga!

I ended up setting the switch programmatically in Program.cs:

// Enable 206 Partial Content responses to enable Video Seeking from api/videos/{id}/file,
// as per, https://github.com/aspnet/Mvc/pull/6895#issuecomment-356477675.
// Should be able to remove this switch and use the enableRangeProcessing overload of File once 
// ASP.NET Core 2.1 released
AppContext.SetSwitch("Switch.Microsoft.AspNetCore.Mvc.EnableRangeProcessing", true);

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants