Uh oh!
There was an error while loading. Please reload this page.
Proper 304 response - #1121
Conversation
mention-bot
commented
Aug 29, 2016
@rullzer, thanks for your PR! By analyzing the annotation information on this pull request, we identified @PVince81, @LukasReschke and @nickvergessen to be potential reviewers |
rullzer
commented
Aug 29, 2016
What I need here is a very simple IFileRead interface or whatever it needs the functions:
As said I'd rather not use a 'File' because of #1039. I'd rather define a new interface that then is implemented by File and stuff we add in SimpleFS as well. |
| try { | ||
| $avatar = $this->avatarManager->getAvatar($userId)->getFile($size); | ||
| $resp = new DataDisplayResponse($avatar->getContent(), | ||
| $resp = new Http\FileDisplayResponse($avatar, |
There was a problem hiding this comment.
Please fully import the class
icewind1991
commented
Aug 29, 2016
Approach looks good |
rullzer
commented
Aug 30, 2016
Ah of course the object inheritance of php is lacking. So lets get this in in the current form. And if we have the simpleFS I'll update the response. |
| */ | ||
| public function callback(IOutput $output) { | ||
| if ($output->getHttpResponseCode() !== Http::STATUS_NOT_MODIFIED) { | ||
| $output->setOutput($this->file->getContent()); |
There was a problem hiding this comment.
@icewind1991: @BernhardPosselt noted here that this might not be very efficient. We might want to use something like http://php.net/manual/en/function.fpassthru.php
But i'd say that is a later optimization
There was a problem hiding this comment.
On the filesystem side we can already do $this->file->fopen('r'), so we just need to have the appframework be able to handle streams as output
There was a problem hiding this comment.
Yeah fair enough. But like I say I'll do that later. This is already much better then the current stuff.
Todo:
|
rullzer
commented
Aug 31, 2016
Review time! |
MorrisJobke
commented
Sep 1, 2016
Somehow the content length is not send for the big preview of the avatar in the personal settings. beside that everything works fine 👍 |
rullzer
commented
Sep 1, 2016
@MorrisJobke I think that is a separate issue. |
rullzer
commented
Sep 1, 2016
@MorrisJobke ah good catch I need to set it! |
rullzer
commented
Sep 1, 2016
@MorrisJobke fixed. Please retest. |
MorrisJobke
commented
Sep 2, 2016
Tested and works 👍 |
rullzer
commented
Sep 5, 2016
@icewind1991 is #1121 (comment) a 👍 from you as well ;)? |
A lazy implementation of the DisplayResponse that only hits the filesystem if the etag and mtime do not match.
rullzer
commented
Sep 5, 2016
Rebased since #1158 was merged |
MorrisJobke
commented
Sep 5, 2016
@LukasReschke mind a review? |
LukasReschke
commented
Sep 5, 2016
LGTM |
The normal DataDisplayResponse works as by getting the Data. And then in the final step of dispatcher checks if the ETag (or modified since) match. However by this point we have already done the reads from the filesystem etc.
This reponse takes a different approach. We provide it with a File object. And only if we need the data (so it isn't a 304) we read it.
We can use this for controllers that return DisplayResponses like the avatars and previews.
Simple POC that it actually works.
Mandatory blackfire trace: https://blackfire.io/profiles/compare/f99e0ddd-c668-40b0-a62b-10c53feba509/graph
Here I used @icewind1991 files_slow app to simulate slow storage (my SSD is to fast). This adds a 50ms delay to all file operations. As you can clearly see even tough both return a 304 the FileDisplayResponse is much faster.
TODO:
intergrate with Discussion SimpleFS (avatar/preview store) #1039 maybe we need a new interface (SimpleFile?) or something. Since we only need a very limited set of actions here.CC: @icewind1991@BernhardPosselt@LukasReschke any feedback?