-
Notifications
You must be signed in to change notification settings - Fork 544
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FileController fix #980
FileController fix #980
Conversation
PavelVsl
commented
Dec 6, 2020
- 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
- using PhysicalFile framework method (current implementation causes file locks and 500 error at heavy load) - Add correct mimetype to header based on file extension
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:
It was modified to stream the file directly which during testing seemed to resolve the prior issues.
Are you saying that the new logic still has performance issues? Is there a way to reproduce the issue? |
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? |
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.
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.
|
"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? |
Do you have any thoughts on the content header ( ie. inline vs attachment ) item? |
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 ? |
@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. |
We can add optional parameter to controller |
@chlupac I agree with your suggestion. We would also want a new overload method in the FileService to call this API. |
@chlupac do you want me to merge this PR and we can implement content-disposition changes in a separate PR? |
pls, merge it, i have to test content-disposition, i will do it in separate PR |