FileController fix

[PavelVsl]

• using PhysicalFile framework method for serving file (current implementation causes file locks and 500 error at heavy load)
• Add correct mimetype to header based on file extension

[sbwalker]

Previously the FileController would first read a file into memory and then send the file in the response, which caused performance issues with large files and heavy load:

                    byte[] filebytes = System.IO.File.ReadAllBytes(filepath);
                    return File(filebytes, "application/octet-stream", file.Name);

It was modified to stream the file directly which during testing seemed to resolve the prior issues.

                    var stream = new FileStream(filepath, FileMode.Open);
                    return File(stream, "application/octet-stream", file.Name);

Are you saying that the new logic still has performance issues? Is there a way to reproduce the issue?

[sbwalker]

The improvement to set the MIME type is very useful.

One other suggestion which @iJungleboy made previously is that the FileController is setting the 'attachment' content header rather than the 'inline' content header. 'Inline' indicates that the entity should be immediately displayed to the user, whereas 'attachment' means that the user should take additional action to view the entity ( and the `filename' parameter is used to suggest a filename).

The suggestion is that Oqtane should use:

return File(stream, mimetype);

rather than:

return File(stream, mimetype, file.Name);

as the latter version sets the 'attachment' content header. I am not sure what impact this has, if any? Does anyone know if this has an affect on application performance - ie. perhaps the impact varies by browser?

[PavelVsl]

It's hard to reproduce, it happens on live server when serving lot of images. Sometimes image is not displayed and server returns 500, because file was locked.

var stream = new FileStream(filepath, FileMode.Open,FileAccess.Read,FileShare.ReadWrite)

should be used to open file without lock

When I replace stream reading with framework function problem disappear. I do not see reason why reinvent wheel and not use this standard controller method.

return PhysicalFile(filepath, file.Name.GetMimeType(), file.Name);

[sbwalker]

"I do not see reason why reinvent wheel and not use this standard controller method." - I agree - I was just trying to understand the problem as I thought it had been resolved previously.

Do you have any thoughts on the content header ( ie. inline vs attachment ) item?

[PavelVsl]

Do you have any thoughts on the content header ( ie. inline vs attachment ) item?
Not sure, I have no experience with this theme.

[sbwalker]

It appears that the HTTP Protocol Specification specifies that is a response has headers which contain "content-disposition: attachment" the file should be treated as an attachment and trigger the download dialog. However each browser implements this differently... most modern browsers evaluate the context for the file request to determine whether to display it inline or trigger a download dialog. For example an IMG tag in a web page requesting a JPEG file will render the file inline, even if the response headers indicate "content-disposition: attachment". Does this sound correct @iJungleboy ?

[iJungleboy]

@sbwalker That's about correct. Basically nothing should have real download headers unless it should be downloaded. For example, PDFs will also behave differently - in one case it's shown, in another it's downloaded.

Without going into the exact changes here - I believe Oqtane should offer both variations but inline should be the default, as most assets will be used in the page - even PDFs. A real download should only be triggered when really wanted - so the api-call should be different for that.

Note also that a newer chrome can set the download-information in the link https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Disposition but in general I think the API should continue to handle the different scenarios.

[PavelVsl]

We can add optional parameter to controller
/file/download/444/inline (default, same as /file/download/444)
/file/download/444/attach
and let module developer decide

[sbwalker]

@chlupac I agree with your suggestion. We would also want a new overload method in the FileService to call this API.

[sbwalker]

@chlupac do you want me to merge this PR and we can implement content-disposition changes in a separate PR?

[PavelVsl]

pls, merge it, i have to test content-disposition, i will do it in separate PR