Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-39832

Send strong ETags for partial content

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.3.9, 2.4.6, 2.5.2, 2.6
    • Component/s: Files API
    • Labels:
    • Testing Instructions:
      Hide

      Apply the same testing instruction described in MDL-39688 except for:

      1. Now ETag header must be always present, in the form of Etag: "<content hash>". Please note that the case Moodle adopted for the ETag header is with T being in lower case.
      2. Chrome must be used first, especially to check the combination with $CFG->xsendfile: the issue has been found when playing MP4 files in Chrome under some combinations of web deployments where X-SendFile was used for performance reasons. The most simple combination is: Apache/2 + mod_xsendfile + $CFG->xsendfile = 'X-Sendfile'; in config.php
      3. In order to verify that caching capabilities work as expected you could use one of the MP4 files used to investigate the issue e.g.:
      4. Do not launch them directly to perform the tests by means of using the File resource link (e.g.: http://vm-centos5/moodle-master/mod/resource/view.php?id=51) but discover the file that will be served by the Files API looking for this URL http://hostname/path/to/moodle/pluginfile.php/<id>/mod_resource/content/<courseid>/<filename> in the media plugin HTML code block (look at the HTML source), e.g.: http://vm-centos5/moodle-master/pluginfile.php/78/mod_resource/content/1/mi2_vorbis51.mp4
      5. Before launching the URL above, please open Fiddler and press F12 to start "capturing" the network traffic
      6. Open the browser, Chrome, press F12 and select the Network tab
      7. Copy the URL above to let Chrome play the content 'till the end: you'll see in both Fiddler and Network tab the full and partial request according with your eventual playing with the slider, Content-Length will change according with Range requested in case of HTTP 206. In case of testing a local instance, the bigger (> 10MB) the movie the higher the chance to see many different HTTP partial requests when moving the slider forward, over the buffered content and then back again, untill the file will be fully buffered (then cached)
      8. Cleanup Fiddler capturing
      9. Close Chrome and then reopen it, press F12, select the Network tab, paste the URL, press ENTER
      10. Move the slider up and down to see the partial request being performed and the content being shown from that point of the slider. When the content will be cached, subsequent request will be served from browser cache instead of making requests to the server i.e. Fiddler will register nothing and the Network tab in Chrome will report request being served from the cache
      Show
      Apply the same testing instruction described in MDL-39688 except for: Now ETag header must be always present, in the form of Etag: "<content hash>" . Please note that the case Moodle adopted for the ETag header is with T being in lower case. Chrome must be used first, especially to check the combination with $CFG->xsendfile : the issue has been found when playing MP4 files in Chrome under some combinations of web deployments where X-SendFile was used for performance reasons. The most simple combination is: Apache/2 + mod_xsendfile + $CFG->xsendfile = 'X-Sendfile'; in config.php In order to verify that caching capabilities work as expected you could use one of the MP4 files used to investigate the issue e.g.: http://samples.mplayerhq.hu/MPEG-4/vorbis-in-mp4/mi2_vorbis51.mp4 http://www.tools4movies.com/dvd_catalyst_profile_samples/The%20Amazing%20Spiderman%20bionic%20hq.mp4 Download them e.g. using wget and upload each of them in Moodle as a File resource . Do not launch them directly to perform the tests by means of using the File resource link (e.g.: http://vm-centos5/moodle-master/mod/resource/view.php?id=51 ) but discover the file that will be served by the Files API looking for this URL http://hostname/path/to/moodle/pluginfile.php/ <id>/mod_resource/content/<courseid>/<filename> in the media plugin HTML code block (look at the HTML source), e.g.: http://vm-centos5/moodle-master/pluginfile.php/78/mod_resource/content/1/mi2_vorbis51.mp4 Before launching the URL above, please open Fiddler and press F12 to start "capturing" the network traffic Open the browser, Chrome, press F12 and select the Network tab Copy the URL above to let Chrome play the content 'till the end: you'll see in both Fiddler and Network tab the full and partial request according with your eventual playing with the slider, Content-Length will change according with Range requested in case of HTTP 206 . In case of testing a local instance, the bigger (> 10MB) the movie the higher the chance to see many different HTTP partial requests when moving the slider forward, over the buffered content and then back again, untill the file will be fully buffered (then cached) Cleanup Fiddler capturing Close Chrome and then reopen it, press F12, select the Network tab, paste the URL, press ENTER Move the slider up and down to see the partial request being performed and the content being shown from that point of the slider. When the content will be cached, subsequent request will be served from browser cache instead of making requests to the server i.e. Fiddler will register nothing and the Network tab in Chrome will report request being served from the cache
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      m26_MDL-39832_Fix_Chrome_Issues_ETag_XSendfile_2ndRound

      Description

      See MDL-39688, this patch started to implement ETag headers based on HTTP ranges.

      Further investigations have driven to another solution which should be applied to the branches where MDL-39688 has been applied because of the ETag header added by Moodle is content driven, helping browser caching mechanisms to work as expected even in cluster deployments where e.g. ETag being added by Apache+X-Sendifle could be built on top of file inodes, probably dependent on the web server serving the request even if files are shared ($CFG->dataroot) someway between all the nodes.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            skodak Petr Skoda added a comment -

            Please test...

            Show
            skodak Petr Skoda added a comment - Please test...
            Hide
            dobedobedoh Andrew Nicols added a comment -

            I'll look at this for peer review tomorrow. Had a quick glance now and everything looks good.

            Show
            dobedobedoh Andrew Nicols added a comment - I'll look at this for peer review tomorrow. Had a quick glance now and everything looks good.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Oh, one comment. Is it worth making the $etag parameter optional and defaulting to null or ''? This will break compatibility for any plugins out there serving files themselves for some reason.

            Show
            dobedobedoh Andrew Nicols added a comment - Oh, one comment. Is it worth making the $etag parameter optional and defaulting to null or ''? This will break compatibility for any plugins out there serving files themselves for some reason.
            Hide
            skodak Petr Skoda added a comment -

            Hi, yes, I was considering that too, but because we can not print any debug messages and the error output must be already disabled in these scripts via "define('NO_DEBUG_DISPLAY', true);" this should not create any problems, it is just a notice that will go to error_log, right?

            Show
            skodak Petr Skoda added a comment - Hi, yes, I was considering that too, but because we can not print any debug messages and the error output must be already disabled in these scripts via "define('NO_DEBUG_DISPLAY', true);" this should not create any problems, it is just a notice that will go to error_log, right?
            Hide
            dobedobedoh Andrew Nicols added a comment -

            I agree with you Re the default parameter value - that makes some sense, though because we do set NO_DEBUG_DISPLAY, it may mean that plugin developer may never discover the bug :\

            [Y] Syntax
            [Y] Output
            [Y] Whitespace
            [-] Language
            [-] Databases
            [N] Testing
            [Y] Security
            [-] Documentation
            [Y] Git
            [N] Sanity check

            Testing

            Needs some testing instructions

            Sanity check

            General:
            Just wondering why you're dropping both ETag and Etag? According to RFC 2616 Section 4.2, "Field names are case-insensitive.". The documentation on header_remove also states that the parameter name is case insensitive.

            lib/xsendfile.php:
            Also I think it would make more sense to move your new header_etag function out of xsendfilelub.php and into somewhere more appropriate - perhaps lib/weblib.php. Sending etags is not related to XSendFile really.

            I guess it makes sense to use header rather than header_etag() when you know that there won't be any byte serving in use.

            Other than the points above, this looks good and a much welcomed addition

            Show
            dobedobedoh Andrew Nicols added a comment - I agree with you Re the default parameter value - that makes some sense, though because we do set NO_DEBUG_DISPLAY, it may mean that plugin developer may never discover the bug :\ [Y] Syntax [Y] Output [Y] Whitespace [-] Language [-] Databases [N] Testing [Y] Security [-] Documentation [Y] Git [N] Sanity check Testing Needs some testing instructions Sanity check General: Just wondering why you're dropping both ETag and Etag? According to RFC 2616 Section 4.2, "Field names are case-insensitive.". The documentation on header_remove also states that the parameter name is case insensitive. lib/xsendfile.php: Also I think it would make more sense to move your new header_etag function out of xsendfilelub.php and into somewhere more appropriate - perhaps lib/weblib.php. Sending etags is not related to XSendFile really. I guess it makes sense to use header rather than header_etag() when you know that there won't be any byte serving in use. Other than the points above, this looks good and a much welcomed addition
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Or was that a request for me to test and write instructions?

            Show
            dobedobedoh Andrew Nicols added a comment - Or was that a request for me to test and write instructions?
            Hide
            skodak Petr Skoda added a comment -

            Grrr - I read header_remove() was case-sensitive, fixing...

            header_etag() can not be in core libs because it is in pages that do not have any core libs available, all they have is CFG from config.php

            I think about the hader_etag() once more, but so far I did not find better place for it.

            Thanks!

            Show
            skodak Petr Skoda added a comment - Grrr - I read header_remove() was case-sensitive, fixing... header_etag() can not be in core libs because it is in pages that do not have any core libs available, all they have is CFG from config.php I think about the hader_etag() once more, but so far I did not find better place for it. Thanks!
            Hide
            skodak Petr Skoda added a comment -

            Repo updated, I have used the name "_header_etag" to mark it private and I have also added some more inline docs. The tag removing is now fixed too. Thanks a lot.

            Show
            skodak Petr Skoda added a comment - Repo updated, I have used the name "_header_etag" to mark it private and I have also added some more inline docs. The tag removing is now fixed too. Thanks a lot.
            Hide
            matteo Matteo Scaramuccia added a comment -

            Hi All,
            I'm keep on reading RFCs and some code: my last comment has been written in https://tracker.moodle.org/browse/MDL-39688?focusedCommentId=224831&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-224831.

            I must admit that I'm somewhat confused, sad for my ignorance: right now I'm thinking the opposite compared to my own comment linked above that is to say I'm thinking the same way as Andrew does when he quotes http://tools.ietf.org/html/rfc2616#section-13.5.4: ETag must be always related to the full content representing the URL being requested, served partially or not.

            To enforce Andrew's point http://tools.ietf.org/html/rfc2616#section-14.27 is more explicit about the ETag to be related with the entire content and not to the partial response, indeed If-Range should do the trick for an outdated partial response.
            But... we've evidence that ETag for partial content breaks things as per Andrew's use case: I'm totally lost... unless there is an issue in the way that MP4 client is able to cache partial responses.

            ETag in Moodle will always be strong being equal to the content hash and when the content change ETag will change too. Should Moodle implement the support for $_SERVER['HTTP_IF_RANGE'] too (limited or not to X-Sendfile), re-sending the whole file in case of mismatching?
            Maybe sniffing the traffic to reconstruct HTTP Requests and HTTP Responses between the two parts while generating the issue, as described in MDL-39688, will help.

            My apologise for the noise I've created 'till now.

            Truly,
            Matteo

            Show
            matteo Matteo Scaramuccia added a comment - Hi All, I'm keep on reading RFCs and some code: my last comment has been written in https://tracker.moodle.org/browse/MDL-39688?focusedCommentId=224831&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-224831 . I must admit that I'm somewhat confused, sad for my ignorance: right now I'm thinking the opposite compared to my own comment linked above that is to say I'm thinking the same way as Andrew does when he quotes http://tools.ietf.org/html/rfc2616#section-13.5.4: ETag must be always related to the full content representing the URL being requested, served partially or not. To enforce Andrew's point http://tools.ietf.org/html/rfc2616#section-14.27 is more explicit about the ETag to be related with the entire content and not to the partial response, indeed If-Range should do the trick for an outdated partial response. But... we've evidence that ETag for partial content breaks things as per Andrew's use case: I'm totally lost... unless there is an issue in the way that MP4 client is able to cache partial responses. ETag in Moodle will always be strong being equal to the content hash and when the content change ETag will change too. Should Moodle implement the support for $_SERVER ['HTTP_IF_RANGE'] too (limited or not to X-Sendfile ), re-sending the whole file in case of mismatching? Maybe sniffing the traffic to reconstruct HTTP Requests and HTTP Responses between the two parts while generating the issue, as described in MDL-39688 , will help. My apologise for the noise I've created 'till now. Truly, Matteo
            Hide
            skodak Petr Skoda added a comment -

            I am also confused a bit after reading the spec, the reality in proxies and client might be different unfortunately.

            I am not sure my proposed patch is the correct solution, this was the main reason why I created this issue, I do not think we should rush this through integration now.

            Thanks everybody!

            Show
            skodak Petr Skoda added a comment - I am also confused a bit after reading the spec, the reality in proxies and client might be different unfortunately. I am not sure my proposed patch is the correct solution, this was the main reason why I created this issue, I do not think we should rush this through integration now. Thanks everybody!
            Hide
            dobedobedoh Andrew Nicols added a comment -

            I'm glad that I'm not the only one confused then Matteo Scaramuccia!

            I've just tried your patch Petr, and it still doesn't work as expected - MP4 only plays within Chrome when we don't return any ETag headers with the byte-served content it seems.
            Still investigating.

            Show
            dobedobedoh Andrew Nicols added a comment - I'm glad that I'm not the only one confused then Matteo Scaramuccia ! I've just tried your patch Petr, and it still doesn't work as expected - MP4 only plays within Chrome when we don't return any ETag headers with the byte-served content it seems. Still investigating.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            This only seems to affect when using X-Accel-Redirect (and possibly other X-Sendfile implementations). It does not seem to affect the stock Moodle byte-serving.

            Show
            dobedobedoh Andrew Nicols added a comment - This only seems to affect when using X-Accel-Redirect (and possibly other X-Sendfile implementations). It does not seem to affect the stock Moodle byte-serving.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            In fact, it's fine with nginx proxying to an apache server right up until you enable the X-Sendfile setting.

            Show
            dobedobedoh Andrew Nicols added a comment - In fact, it's fine with nginx proxying to an apache server right up until you enable the X-Sendfile setting.
            Hide
            dobedobedoh Andrew Nicols added a comment - - edited

            Just to confirm, this is also broken with mod_xsendfile on Apache with ETags present both with, and without the range. That's just straight Apache2 -> mod_php5 so no proxying going on.

            Show
            dobedobedoh Andrew Nicols added a comment - - edited Just to confirm, this is also broken with mod_xsendfile on Apache with ETags present both with, and without the range. That's just straight Apache2 -> mod_php5 so no proxying going on.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            The only browser I've been able to test in so far is Chrome. It seems that neither Firefox, nor Opera seem to request partial content for mp4 content (grr) so they don't seem to exhibit this bug (yay). They may of course request partial content for other content types but I've not tried that.

            Matteo Scaramuccia and Petr Skoda, what browsers + content are you testing this with?

            Show
            dobedobedoh Andrew Nicols added a comment - The only browser I've been able to test in so far is Chrome. It seems that neither Firefox, nor Opera seem to request partial content for mp4 content (grr) so they don't seem to exhibit this bug (yay). They may of course request partial content for other content types but I've not tried that. Matteo Scaramuccia and Petr Skoda , what browsers + content are you testing this with?
            Hide
            matteo Matteo Scaramuccia added a comment - - edited

            Hi Andrew,
            'till now I've just read specs and code, nothing more: I was waiting for any feedback about what would be the client showing that strange and blocking issue.
            I'll play with CR and MP4 plus I'd create some tests using curl to verify Moodle supporting byte serving - which is by far working "on average" - even with If-Range - currently missing, I'm thinking to propose its support for 2.6 - and I'll look at the HTTP Headers exchanged using CR, to identify what could be the issue in terms of HTTP specs compliance or CR specific issue. Unfortunately this work will be done on spare time, typically evening.

            EDIT @20130528T0925+02:00: have you already checked that the byte range provided by CR is a real partial response request i.e. not what described here, http://stackoverflow.com/questions/12801192/client-closes-connection-when-streaming-m4v-from-apache-to-chrome-with-jplayer#answer-12979414? It could be where Moodle byte serving could be improved in replying to a "fake Partial Response" request.

            Show
            matteo Matteo Scaramuccia added a comment - - edited Hi Andrew, 'till now I've just read specs and code, nothing more: I was waiting for any feedback about what would be the client showing that strange and blocking issue. I'll play with CR and MP4 plus I'd create some tests using curl to verify Moodle supporting byte serving - which is by far working "on average" - even with If-Range - currently missing, I'm thinking to propose its support for 2.6 - and I'll look at the HTTP Headers exchanged using CR, to identify what could be the issue in terms of HTTP specs compliance or CR specific issue. Unfortunately this work will be done on spare time, typically evening. EDIT @ 20130528T0925+02:00 : have you already checked that the byte range provided by CR is a real partial response request i.e. not what described here, http://stackoverflow.com/questions/12801192/client-closes-connection-when-streaming-m4v-from-apache-to-chrome-with-jplayer#answer-12979414? It could be where Moodle byte serving could be improved in replying to a "fake Partial Response" request.
            Hide
            skodak Petr Skoda added a comment - - edited

            Matteo: Do you want this to be assigned to you? I am not sure I would find enough time to create a solution based on reverse engineering of proxies and clients.

            Or Andrew, do you want this?

            Show
            skodak Petr Skoda added a comment - - edited Matteo: Do you want this to be assigned to you? I am not sure I would find enough time to create a solution based on reverse engineering of proxies and clients. Or Andrew, do you want this?
            Hide
            matteo Matteo Scaramuccia added a comment -

            Hi Petr,
            my time is unfortunately limited, maybe Andrew's time for this issue could be more than mine: I'll be happy to help in any case as per my capabilities and available time.

            Show
            matteo Matteo Scaramuccia added a comment - Hi Petr, my time is unfortunately limited, maybe Andrew's time for this issue could be more than mine: I'll be happy to help in any case as per my capabilities and available time.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            I can try and look into this further in about two weeks time.

            Show
            dobedobedoh Andrew Nicols added a comment - I can try and look into this further in about two weeks time.
            Hide
            skodak Petr Skoda added a comment -

            Here is my patch, anybody is free to work on this issue: https://github.com/skodak/moodle/compare/wip_MDL-39832_m26_etagranges

            Thanks everybody!

            Show
            skodak Petr Skoda added a comment - Here is my patch, anybody is free to work on this issue: https://github.com/skodak/moodle/compare/wip_MDL-39832_m26_etagranges Thanks everybody!
            Hide
            matteo Matteo Scaramuccia added a comment - - edited

            Hi Andrew,
            this weekend I'll do some investigations&coding, could you provide me a link to an MP4 sample file "big" enough to replicate the issue using Chrome on master running under Apache 2 with mod_xsendfile?
            One other potential issue is that ETag must be double quoted, http://tools.ietf.org/html/rfc2616#section-3.11:

                  entity-tag = [ weak ] opaque-tag
                  weak       = "W/"
                  opaque-tag = quoted-string
            

            Even if I'll be unsuccessful I'll provide a WIP branch to show how the things should be IMHO changed according with RFC2616.

            EDIT: in the mean time I've found some MP4 files here, http://www.thriveforums.org/forum/toshiba-thrive-video/3418-sample-mp4-videos-thrive.html, converted from MOV files.

            TIA,
            Matteo

            Show
            matteo Matteo Scaramuccia added a comment - - edited Hi Andrew, this weekend I'll do some investigations&coding, could you provide me a link to an MP4 sample file "big" enough to replicate the issue using Chrome on master running under Apache 2 with mod_xsendfile ? One other potential issue is that ETag must be double quoted, http://tools.ietf.org/html/rfc2616#section-3.11: entity-tag = [ weak ] opaque-tag weak = "W/" opaque-tag = quoted-string Even if I'll be unsuccessful I'll provide a WIP branch to show how the things should be IMHO changed according with RFC2616. EDIT: in the mean time I've found some MP4 files here, http://www.thriveforums.org/forum/toshiba-thrive-video/3418-sample-mp4-videos-thrive.html , converted from MOV files. TIA, Matteo
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Hi Matteo,

            I'm not using anything special - I'm just using a free sample MP4 I found somewhere. It doesn't have to be anything long. I uploaded it to a section description, then viewed it in Chrome using the link it displays as to entirely rule out an issue with the Media plugin. I.e. I'm opening pluginfile.php directly. Make sure you've not disabled the browser cache (in Chrome Dev tools).

            Show
            dobedobedoh Andrew Nicols added a comment - Hi Matteo, I'm not using anything special - I'm just using a free sample MP4 I found somewhere. It doesn't have to be anything long. I uploaded it to a section description, then viewed it in Chrome using the link it displays as to entirely rule out an issue with the Media plugin. I.e. I'm opening pluginfile.php directly. Make sure you've not disabled the browser cache (in Chrome Dev tools).
            Hide
            matteo Matteo Scaramuccia added a comment -

            Hi All,
            I've done several experiments using Apache/2.2.3, mod_xsendfile/0.12 and Chrome with the provided MP4 files (http://samples.mplayerhq.hu/MPEG-4/vorbis-in-mp4/mi2_vorbis51.mp4 included) changing things as per my previous comments ending with adding double quotes: this last change did the trick.

            @Andrew Nicols: could you test it?
            My suggestion is to apply this change in all the branches where MDL-39688 has been applied: I'll be happy to provide them including test instructions.

            Show
            matteo Matteo Scaramuccia added a comment - Hi All, I've done several experiments using Apache/2.2.3 , mod_xsendfile/0.12 and Chrome with the provided MP4 files ( http://samples.mplayerhq.hu/MPEG-4/vorbis-in-mp4/mi2_vorbis51.mp4 included) changing things as per my previous comments ending with adding double quotes: this last change did the trick. @ Andrew Nicols : could you test it? My suggestion is to apply this change in all the branches where MDL-39688 has been applied: I'll be happy to provide them including test instructions.
            Hide
            skodak Petr Skoda added a comment -

            Wow, is it really only the missing double quotes? If yes should we fix the rest of etags? Are there any other headers that require double quotes?

            Show
            skodak Petr Skoda added a comment - Wow, is it really only the missing double quotes? If yes should we fix the rest of etags? Are there any other headers that require double quotes?
            Hide
            matteo Matteo Scaramuccia added a comment - - edited

            Hi Petr,
            it looks like that: would you like to test it too, reverting first MDL-36888 to see that Chrome will not be happy and then applying my proposal? In my environment works as described, but you know, I could be wrong
            I'll double check RFC2616, while waiting for comments from Andrew, for other potential syntax issues.

            Quick searched for ((^|\s)header([^LH]+); - No redirect (Location) and HTTP/1.0 statuses -, here the comments on the matches:

            • Accept-Ranges: no quotes
            • Cache-Control: no quotes, just for extensions. Not applicable in Moodle
            • Content-Disposition, filename must be quoted: not always in Moodle e.g. grade/edit/outcome/export.php
            • Content-Encoding: no quotes
            • Content-Length: no quotes
            • Content-Type: no quotes. Almost OK except for:
              • LF in lib/tablelib.php. Same in mod/survey/download.php
              • charset should not be quoted e.g. not Content-Type: text/xml; charset="utf-8"': few files affected
            • Etag: quotes required. Missing quotes in: lib/csslib.php, theme/image.php, theme/yui_image.php, ...
            • Expires: no quotes
            • Pragma: no quotes except for extensions. Not applicable in Moodle
            • Status: no quotes
            • Last-Modified: no quotes (HTTP-date)
            • WWW-Authenticate: quotes for Realm (RFC2617)

            If Andrew and you will confirm my current proposal to work as expected in your environments, I'd suggest to fix all the other ETag headers, e.g. in lib/csslib.php, as part of a separate improvement for 2.6, not sure for the previous versions.
            Note: FirePHP::setHeader doesn't apply quotes for headers BUT it is not used to set ETag. Similar comment for XHProf wrapper.

            Show
            matteo Matteo Scaramuccia added a comment - - edited Hi Petr, it looks like that: would you like to test it too, reverting first MDL-36888 to see that Chrome will not be happy and then applying my proposal? In my environment works as described, but you know, I could be wrong I'll double check RFC2616 , while waiting for comments from Andrew, for other potential syntax issues. Quick searched for ((^|\s)header( [^LH] +); - No redirect ( Location ) and HTTP/1.0 statuses -, here the comments on the matches: Accept-Ranges : no quotes Cache-Control : no quotes, just for extensions. Not applicable in Moodle Content-Disposition , filename must be quoted: not always in Moodle e.g. grade/edit/outcome/export.php Content-Encoding : no quotes Content-Length : no quotes Content-Type : no quotes. Almost OK except for: LF in lib/tablelib.php . Same in mod/survey/download.php charset should not be quoted e.g. not Content-Type: text/xml; charset="utf-8"' : few files affected Etag : quotes required. Missing quotes in: lib/csslib.php , theme/image.php , theme/yui_image.php , ... Expires : no quotes Pragma : no quotes except for extensions. Not applicable in Moodle Status : no quotes Last-Modified : no quotes ( HTTP-date ) WWW-Authenticate : quotes for Realm (RFC2617) If Andrew and you will confirm my current proposal to work as expected in your environments, I'd suggest to fix all the other ETag headers, e.g. in lib/csslib.php , as part of a separate improvement for 2.6, not sure for the previous versions. Note: FirePHP::setHeader doesn't apply quotes for headers BUT it is not used to set ETag . Similar comment for XHProf wrapper.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Thanks Matteo - I can't believe it was this simple. I've spent a couple of hours trying to track it down and re-read the RFC numerous times. Kudos

            I've just tried wrapping in quotes and it does indeed work as expected. Your patch looks good to me.

            I think that we should correct the quotes in the other ETag headers you've identified as part of this patch. The Byte Range issues are just a symptom of the real issue. This should also be backported to all stable branches IMO.

            I think we should file a new issue for the other header quoting issues you've identified rather than as part of this one.

            Andrew

            Show
            dobedobedoh Andrew Nicols added a comment - Thanks Matteo - I can't believe it was this simple. I've spent a couple of hours trying to track it down and re-read the RFC numerous times. Kudos I've just tried wrapping in quotes and it does indeed work as expected. Your patch looks good to me. I think that we should correct the quotes in the other ETag headers you've identified as part of this patch. The Byte Range issues are just a symptom of the real issue. This should also be backported to all stable branches IMO. I think we should file a new issue for the other header quoting issues you've identified rather than as part of this one. Andrew
            Hide
            skodak Petr Skoda added a comment -

            +1 for the backport

            Show
            skodak Petr Skoda added a comment - +1 for the backport
            Hide
            matteo Matteo Scaramuccia added a comment -

            OK Andrew&Petr, will do one of my evening (CEST).

            Show
            matteo Matteo Scaramuccia added a comment - OK Andrew&Petr, will do one of my evening (CEST).
            Hide
            matteo Matteo Scaramuccia added a comment -

            Hi All,
            here are the backports of the current proposal.

            Do you agree that fixing the other missing quotes could be done in a separate issue? For clarity sake, this issue should IMHO cover just what discussed and done in MDL-39688 but I'll wait for your thoughts.

            Show
            matteo Matteo Scaramuccia added a comment - Hi All, here are the backports of the current proposal. Do you agree that fixing the other missing quotes could be done in a separate issue? For clarity sake, this issue should IMHO cover just what discussed and done in MDL-39688 but I'll wait for your thoughts.
            Hide
            skodak Petr Skoda added a comment -

            submitting, +1 for separate issue for the remaining quotes, thanks!!

            Show
            skodak Petr Skoda added a comment - submitting, +1 for separate issue for the remaining quotes, thanks!!
            Hide
            poltawski Dan Poltawski added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

            TIA and ciao

            Show
            poltawski Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
            Hide
            matteo Matteo Scaramuccia added a comment -

            Rebased.

            Show
            matteo Matteo Scaramuccia added a comment - Rebased.
            Hide
            poltawski Dan Poltawski added a comment -

            Great detective work Matteo!

            I've integrated this to master, 25, 24 and 23 - thanks!

            Show
            poltawski Dan Poltawski added a comment - Great detective work Matteo! I've integrated this to master, 25, 24 and 23 - thanks!
            Hide
            ankit_frenz Ankit Agarwal added a comment - - edited

            The file doesn't play any more on chrome. The content of file is empty always. This works perfectly on stable.
            Let me know if I can provide more info on anything.
            Dan has confirmed the issue.
            Failing.

            Show
            ankit_frenz Ankit Agarwal added a comment - - edited The file doesn't play any more on chrome. The content of file is empty always. This works perfectly on stable. Let me know if I can provide more info on anything. Dan has confirmed the issue. Failing.
            Hide
            poltawski Dan Poltawski added a comment -

            To try and add some detail, there are definite scenarios where chrome seems to not load the file when requested directly, but I HAVE seen it load when requested by quicktime. Either way, I have to say it does look to be something fishy with the etag

            Show
            poltawski Dan Poltawski added a comment - To try and add some detail, there are definite scenarios where chrome seems to not load the file when requested directly, but I HAVE seen it load when requested by quicktime. Either way, I have to say it does look to be something fishy with the etag
            Hide
            matteo Matteo Scaramuccia added a comment -

            Hi Ankit&Dan,
            could you give me some details about your tests to let me replicate it in my environment? Here are the questions about what I'm supposing to be required to identify the scope of your tests:

            1. Before starting with tests, did you cleanup Chrome cache? It's unfortunately a missing point in the testing instructions
            2. Could you share the movie used for your tests?
            3. Could you confirm that tests has been performed using the pluginfile.php URL?
            4. Could you confirm that the connection between your browser and the Moodle instance is direct e.g. a local installation i.e. no proxy in between?
            5. Did you use X-Sendfile configuration or just "plain" PHP module connection?
            6. Did you check what both Fiddler and Chrome Dev Tools show during your tests to identify any eventual other reason? (I know, this is the part I must work on... when I'll replicate it )
            Show
            matteo Matteo Scaramuccia added a comment - Hi Ankit&Dan, could you give me some details about your tests to let me replicate it in my environment? Here are the questions about what I'm supposing to be required to identify the scope of your tests: Before starting with tests, did you cleanup Chrome cache? It's unfortunately a missing point in the testing instructions Could you share the movie used for your tests? Could you confirm that tests has been performed using the pluginfile.php URL? Could you confirm that the connection between your browser and the Moodle instance is direct e.g. a local installation i.e. no proxy in between? Did you use X-Sendfile configuration or just "plain" PHP module connection? Did you check what both Fiddler and Chrome Dev Tools show during your tests to identify any eventual other reason? (I know, this is the part I must work on... when I'll replicate it )
            Hide
            poltawski Dan Poltawski added a comment -

            > 1. Before starting with tests, did you cleanup Chrome cache? It's unfortunately a missing point in the testing instructions

            Yes - well I tried in incognito window

            > Could you share the movie used for your tests?

            We used the mp4 in the testing instructions, and I also used http://www.quirksmode.org/html5/videos/big_buck_bunny.mp4

            > Could you confirm that tests has been performed using the pluginfile.php URL?

            Yep

            > Could you confirm that the connection between your browser and the Moodle instance is direct e.g. a local installation i.e. no proxy in between?

            Yep

            > Did you use X-Sendfile configuration or just "plain" PHP module connection?

            No X-Sendfile

            > Did you check what both Fiddler and Chrome Dev Tools show during your tests to identify any eventual other reason? (I know, this is the part I must work on... when I'll replicate it )

            No fiddler here as I don't use windows.

            Show
            poltawski Dan Poltawski added a comment - > 1. Before starting with tests, did you cleanup Chrome cache? It's unfortunately a missing point in the testing instructions Yes - well I tried in incognito window > Could you share the movie used for your tests? We used the mp4 in the testing instructions, and I also used http://www.quirksmode.org/html5/videos/big_buck_bunny.mp4 > Could you confirm that tests has been performed using the pluginfile.php URL? Yep > Could you confirm that the connection between your browser and the Moodle instance is direct e.g. a local installation i.e. no proxy in between? Yep > Did you use X-Sendfile configuration or just "plain" PHP module connection? No X-Sendfile > Did you check what both Fiddler and Chrome Dev Tools show during your tests to identify any eventual other reason? (I know, this is the part I must work on... when I'll replicate it ) No fiddler here as I don't use windows.
            Hide
            poltawski Dan Poltawski added a comment -

            (i'm happy to sniff the traffic to work out whats going on, but I am not I can)

            Show
            poltawski Dan Poltawski added a comment - (i'm happy to sniff the traffic to work out whats going on, but I am not I can)
            Hide
            ankit_frenz Ankit Agarwal added a comment - - edited

            Hi Matteo,
            Not sure am being of much help here, but anyway:-

            1. No, I didn't . But I just retried with clean cache, same issue.
            2. I used the one you mentioned http://samples.mplayerhq.hu/MPEG-4/vorbis-in-mp4/mi2_vorbis51.mp4 and the one here http://www.quirksmode.org/html5/tests/video.html
            3. Yes
            4. Yes
            5. Yes (I did see x send file tag in header)
            6. Am on linux, so used only the dev tools. I couldn't think of any other reason, why the contents will be empty. Anyways here are the headers, see if it helps

              GET /int/master/moodle/pluginfile.php/418/mod_resource/content/1/big_buck_bunny.mp4 HTTP/1.1
              Host: ankit.moodle.local
              Connection: keep-alive
              Cache-Control: max-age=0
              Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
              User-Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/27.0.1453.110 Safari/537.36
              Referer: http://ankit.moodle.local/int/master/moodle/login/index.php
              Accept-Encoding: gzip,deflate,sdch
              Accept-Language: en-GB,en-US;q=0.8,en;q=0.6
              Cookie: MoodleSession=nk41ksu6f8qrggotnbnnbu3pd4; MOODLEID1_=%25DB%25CF%2592%25FBU%25F3%258C; MOODLEID1_=%2598%2529%25B2%25B0%25ED2%252B0
              HTTP/1.1 200 OK
              Date: Thu, 13 Jun 2013 02:17:29 GMT
              Server: Apache/2.2.22 (Ubuntu)
              X-Powered-By: PHP/5.3.10-1ubuntu3.6
              Expires: Fri, 14 Jun 2013 02:17:29 GMT
              Cache-Control: max-age=86400
              Pragma: 
              Content-Disposition: inline; filename="big_buck_bunny.mp4"
              Last-Modified: Wed, 12 Jun 2013 07:33:28 GMT
              Etag: "0181d7880acf0c443a57cd2510a5b81447fa577e"
              Accept-Ranges: bytes
              X-Sendfile: /var/moodledata/int/master/filedir/01/81/0181d7880acf0c443a57cd2510a5b81447fa577e
              Content-Length: 0
              Keep-Alive: timeout=5, max=98
              Connection: Keep-Alive
              Content-Type: video/mp4
              
              

              GET /int/master/moodle/pluginfile.php/418/mod_resource/content/1/big_buck_bunny.mp4 HTTP/1.1
              Host: ankit.moodle.local
              Connection: keep-alive
              Accept-Encoding: identity;q=1, *;q=0
              User-Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/27.0.1453.110 Safari/537.36
              Accept: */*
              Referer: http://ankit.moodle.local/int/master/moodle/pluginfile.php/418/mod_resource/content/1/big_buck_bunny.mp4
              Accept-Language: en-GB,en-US;q=0.8,en;q=0.6
              Cookie: MoodleSession=nk41ksu6f8qrggotnbnnbu3pd4; MOODLEID1_=%25DB%25CF%2592%25FBU%25F3%258C; MOODLEID1_=%2598%2529%25B2%25B0%25ED2%252B0
              Range: bytes=0-
               
              HTTP/1.1 200 OK
              Date: Thu, 13 Jun 2013 02:17:29 GMT
              Server: Apache/2.2.22 (Ubuntu)
              X-Powered-By: PHP/5.3.10-1ubuntu3.6
              Expires: Fri, 14 Jun 2013 02:17:30 GMT
              Cache-Control: max-age=86400
              Pragma: 
              Content-Disposition: inline; filename="big_buck_bunny.mp4"
              Last-Modified: Wed, 12 Jun 2013 07:33:28 GMT
              Etag: "0181d7880acf0c443a57cd2510a5b81447fa577e"
              Accept-Ranges: bytes
              X-Sendfile: /var/moodledata/int/master/filedir/01/81/0181d7880acf0c443a57cd2510a5b81447fa577e
              Content-Length: 0
              Keep-Alive: timeout=5, max=97
              Connection: Keep-Alive
              Content-Type: video/mp4
              
              

              Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - - edited Hi Matteo, Not sure am being of much help here, but anyway:- No, I didn't . But I just retried with clean cache, same issue. I used the one you mentioned http://samples.mplayerhq.hu/MPEG-4/vorbis-in-mp4/mi2_vorbis51.mp4 and the one here http://www.quirksmode.org/html5/tests/video.html Yes Yes Yes (I did see x send file tag in header) Am on linux, so used only the dev tools. I couldn't think of any other reason, why the contents will be empty. Anyways here are the headers, see if it helps GET /int/master/moodle/pluginfile.php/418/mod_resource/content/1/big_buck_bunny.mp4 HTTP/1.1 Host: ankit.moodle.local Connection: keep-alive Cache-Control: max-age=0 Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8 User-Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/27.0.1453.110 Safari/537.36 Referer: http://ankit.moodle.local/int/master/moodle/login/index.php Accept-Encoding: gzip,deflate,sdch Accept-Language: en-GB,en-US;q=0.8,en;q=0.6 Cookie: MoodleSession=nk41ksu6f8qrggotnbnnbu3pd4; MOODLEID1_=%25DB%25CF%2592%25FBU%25F3%258C; MOODLEID1_=%2598%2529%25B2%25B0%25ED2%252B0 HTTP/1.1 200 OK Date: Thu, 13 Jun 2013 02:17:29 GMT Server: Apache/2.2.22 (Ubuntu) X-Powered-By: PHP/5.3.10-1ubuntu3.6 Expires: Fri, 14 Jun 2013 02:17:29 GMT Cache-Control: max-age=86400 Pragma: Content-Disposition: inline; filename="big_buck_bunny.mp4" Last-Modified: Wed, 12 Jun 2013 07:33:28 GMT Etag: "0181d7880acf0c443a57cd2510a5b81447fa577e" Accept-Ranges: bytes X-Sendfile: /var/moodledata/int/master/filedir/01/81/0181d7880acf0c443a57cd2510a5b81447fa577e Content-Length: 0 Keep-Alive: timeout=5, max=98 Connection: Keep-Alive Content-Type: video/mp4 GET /int/master/moodle/pluginfile.php/418/mod_resource/content/1/big_buck_bunny.mp4 HTTP/1.1 Host: ankit.moodle.local Connection: keep-alive Accept-Encoding: identity;q=1, *;q=0 User-Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/27.0.1453.110 Safari/537.36 Accept: */* Referer: http://ankit.moodle.local/int/master/moodle/pluginfile.php/418/mod_resource/content/1/big_buck_bunny.mp4 Accept-Language: en-GB,en-US;q=0.8,en;q=0.6 Cookie: MoodleSession=nk41ksu6f8qrggotnbnnbu3pd4; MOODLEID1_=%25DB%25CF%2592%25FBU%25F3%258C; MOODLEID1_=%2598%2529%25B2%25B0%25ED2%252B0 Range: bytes=0-   HTTP/1.1 200 OK Date: Thu, 13 Jun 2013 02:17:29 GMT Server: Apache/2.2.22 (Ubuntu) X-Powered-By: PHP/5.3.10-1ubuntu3.6 Expires: Fri, 14 Jun 2013 02:17:30 GMT Cache-Control: max-age=86400 Pragma: Content-Disposition: inline; filename="big_buck_bunny.mp4" Last-Modified: Wed, 12 Jun 2013 07:33:28 GMT Etag: "0181d7880acf0c443a57cd2510a5b81447fa577e" Accept-Ranges: bytes X-Sendfile: /var/moodledata/int/master/filedir/01/81/0181d7880acf0c443a57cd2510a5b81447fa577e Content-Length: 0 Keep-Alive: timeout=5, max=97 Connection: Keep-Alive Content-Type: video/mp4 Thanks
            Hide
            matteo Matteo Scaramuccia added a comment -

            TNX for your replies!
            About sniffing and options out of a Windows env: WireShark is The Network Sniffer, regardless the OS. You can inspect everything, please filter the traffic limiting it at least to HTTP otherwise you'll be lost in a sea of packets . You'll find several hints in the network e.g. http://www.eriky.com/2009/01/sniffing-http-headers-with-wireshark.
            In *nix systems, especially when troubleshooting on the server, tcpdump is a great (and powerful as well) CLI.
            Fiddler is a really nice and powerful tool for Windows, focused on HTTP requests, easy to use almost for any user: that's the reason why I suggested it, my fault not to consider other options.

            I'll perform tests&investigations against your video one of the next evening, unfortunately out of this integration week.

            @Ankit Agarwal: when you configure X-Sendfile in a Moodle deployment you need first to install its support in your web deploy: here, you need to install mod_xsendfile since you're running Moodle under Apache. The goal of that extension is, shortly, to:

            • look for the X-Sendfile header
            • pickup the path to your local system
            • REMOVE it from your HTTP Headers otherwise, big disclosure here!
            • serve your file directly, offloading the PHP module i.e. Moodle will not send anything when running under that context, expecting&trusting that the Web Server will do it as per X-Sendfile specs.

            That's why you receive nothing: please, install the module in your Ubuntu e.g. http://blog.brightbox.co.uk/posts/apache-x-sendfile-module-for-ubuntu-hardy

            Show
            matteo Matteo Scaramuccia added a comment - TNX for your replies! About sniffing and options out of a Windows env: WireShark is The Network Sniffer, regardless the OS. You can inspect everything, please filter the traffic limiting it at least to HTTP otherwise you'll be lost in a sea of packets . You'll find several hints in the network e.g. http://www.eriky.com/2009/01/sniffing-http-headers-with-wireshark . In *nix systems, especially when troubleshooting on the server, tcpdump is a great (and powerful as well) CLI. Fiddler is a really nice and powerful tool for Windows, focused on HTTP requests, easy to use almost for any user: that's the reason why I suggested it, my fault not to consider other options. I'll perform tests&investigations against your video one of the next evening, unfortunately out of this integration week. @ Ankit Agarwal : when you configure X-Sendfile in a Moodle deployment you need first to install its support in your web deploy: here, you need to install mod_xsendfile since you're running Moodle under Apache. The goal of that extension is, shortly, to: look for the X-Sendfile header pickup the path to your local system REMOVE it from your HTTP Headers otherwise, big disclosure here! serve your file directly, offloading the PHP module i.e. Moodle will not send anything when running under that context, expecting&trusting that the Web Server will do it as per X-Sendfile specs. That's why you receive nothing: please, install the module in your Ubuntu e.g. http://blog.brightbox.co.uk/posts/apache-x-sendfile-module-for-ubuntu-hardy
            Hide
            poltawski Dan Poltawski added a comment -

            Hi Matteo,

            Yep I know how to sniff the traffic, but I don't know what you want us to look for

            Show
            poltawski Dan Poltawski added a comment - Hi Matteo, Yep I know how to sniff the traffic, but I don't know what you want us to look for
            Hide
            poltawski Dan Poltawski added a comment -

            Anyway, i'm going to need to revert this, because from what I can see from our testing, it is continuing to cause a problem

            Show
            poltawski Dan Poltawski added a comment - Anyway, i'm going to need to revert this, because from what I can see from our testing, it is continuing to cause a problem
            Hide
            poltawski Dan Poltawski added a comment -

            I've reverted this change, sorry we have had to do that.

            Show
            poltawski Dan Poltawski added a comment - I've reverted this change, sorry we have had to do that.
            Hide
            matteo Matteo Scaramuccia added a comment -

            Hi Dan,

            1. Sniffing: I apologize for my misunderstanding your initial point !
              BTW, what should be looked for is the correct match between Request type and given Response. If the Request contains an Accept header, the Response should be exactly related (e.g. right Content-Length, before looking at the body itself) and you need to compare the Chrome Dev Tools network information with the ones sniffed to be sure that, at the end, a Request shown in Chrome Dev Tools as served by the internal cache means no hit to the server unless the previous (partial) requests didn't cover the all content length and Chrome is correctly required to ask the server for the missing parts.
              Have you already try to cleanup your Chrome cache, perform the same test without the PR to see if your behavior is due to the PR i.e. to the usage of ETag?
            2. Integration: no problem, that's the process. I hope to hear news from Ankit tests setup, too.
            Show
            matteo Matteo Scaramuccia added a comment - Hi Dan, Sniffing: I apologize for my misunderstanding your initial point ! BTW, what should be looked for is the correct match between Request type and given Response. If the Request contains an Accept header, the Response should be exactly related (e.g. right Content-Length , before looking at the body itself) and you need to compare the Chrome Dev Tools network information with the ones sniffed to be sure that, at the end, a Request shown in Chrome Dev Tools as served by the internal cache means no hit to the server unless the previous (partial) requests didn't cover the all content length and Chrome is correctly required to ask the server for the missing parts. Have you already try to cleanup your Chrome cache, perform the same test without the PR to see if your behavior is due to the PR i.e. to the usage of ETag ? Integration: no problem, that's the process. I hope to hear news from Ankit tests setup, too.
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Hi Matteo,
            I already had x-send file installed and enabled, but am not sure if it is working correctly, as I just tested without the x-sendfile config and the vid was working fine. Perhaps some configuration setup is missing, I will investigate a little more and get back to you.
            Cheers

            Show
            ankit_frenz Ankit Agarwal added a comment - Hi Matteo, I already had x-send file installed and enabled, but am not sure if it is working correctly, as I just tested without the x-sendfile config and the vid was working fine. Perhaps some configuration setup is missing, I will investigate a little more and get back to you. Cheers
            Hide
            matteo Matteo Scaramuccia added a comment -

            Hi Ankit,

            1. Setup: please check the Apache error log and the way the module has been packaged/configured for Ubuntu i.e. you should find at least these parameters in your configuration: XSendFile on and XSendFilePath /var/moodledata/int/master, the second being important because it gives the request acceptance scope. Please note that the scope should be built using multiple XSendFilePath lines, at least two in order to map both your WWWROOT and DATAROOT.
            2. Tests: could you talk offline with Dan to search for differences between both setups (when using Chrome with no X-Sendfile in Moodle) to eventually return me any feedback for my investigations?

            TIA

            Show
            matteo Matteo Scaramuccia added a comment - Hi Ankit, Setup: please check the Apache error log and the way the module has been packaged/configured for Ubuntu i.e. you should find at least these parameters in your configuration: XSendFile on and XSendFilePath /var/moodledata/int/master , the second being important because it gives the request acceptance scope. Please note that the scope should be built using multiple XSendFilePath lines, at least two in order to map both your WWWROOT and DATAROOT . Tests: could you talk offline with Dan to search for differences between both setups (when using Chrome with no X-Sendfile in Moodle) to eventually return me any feedback for my investigations? TIA
            Hide
            cibot CiBoT added a comment -

            Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            matteo Matteo Scaramuccia added a comment -

            Hi Dan,
            just finished a first test run using all the permutations of the test configurations under Chrome/27.0.1453.110 Windows/Vista SP2 i.e. including incognito windows and running the PR with and without $CFG->xsendfile = 'X-Sendfile'; against the big movie I proposed and your video: all of them successfully.
            I'll try to use Chrome on a VM with Windows XP SP3, to start excluding issues with my dev/test environment.

            Show
            matteo Matteo Scaramuccia added a comment - Hi Dan, just finished a first test run using all the permutations of the test configurations under Chrome/27.0.1453.110 Windows/Vista SP2 i.e. including incognito windows and running the PR with and without $CFG->xsendfile = 'X-Sendfile'; against the big movie I proposed and your video: all of them successfully. I'll try to use Chrome on a VM with Windows XP SP3 , to start excluding issues with my dev/test environment.
            Hide
            matteo Matteo Scaramuccia added a comment -

            Same successful result under Chrome/27.0.1453.110 Windows/XP SP3 using all the permutations of the test fixtures combinations above.

            Stopping here waiting for some feedback from Ankit.

            Show
            matteo Matteo Scaramuccia added a comment - Same successful result under Chrome/27.0.1453.110 Windows/XP SP3 using all the permutations of the test fixtures combinations above. Stopping here waiting for some feedback from Ankit.
            Hide
            matteo Matteo Scaramuccia added a comment - - edited

            Hi Andrew,
            I guess that the next step should be asking for a new peer review to step into another test session since everything is working on my side even under the worst - as per MDL-39688 - case, Chrome + X-Sendfile.

            Wondering if you'd have the time to repeat your tests too. Since MDL-40002 has been integrated, IMHO we should at least add quotes to the current ETag related code, if we are not confident that this patch does the job in helping bandwidth saving in case of Partial Responses (HTTP 206).

            Test summary:

            Tester w/o X-Sendfile w/ X-Sendfile
            Andrew/1° run
            Andrew/2° run
            Ankit/1° run -
            Dan/1° run -
            Matteo/1° Run
            Matteo/2° Run
            Show
            matteo Matteo Scaramuccia added a comment - - edited Hi Andrew, I guess that the next step should be asking for a new peer review to step into another test session since everything is working on my side even under the worst - as per MDL-39688 - case, Chrome + X-Sendfile . Wondering if you'd have the time to repeat your tests too. Since MDL-40002 has been integrated, IMHO we should at least add quotes to the current ETag related code, if we are not confident that this patch does the job in helping bandwidth saving in case of Partial Responses ( HTTP 206 ). Test summary : Tester w/o X-Sendfile w/ X-Sendfile Andrew/1° run Andrew/2° run Ankit/1° run - Dan/1° run - Matteo/1° Run Matteo/2° Run
            Hide
            dobedobedoh Andrew Nicols added a comment -

            I've been trying to get around to testing this myself too for the past few days but haven't had a chance. The last time that I checked, it was all working correctly both with, and without X-SendFile. I also tried with nginx and X-Accel-Redirect just for completeness.

            I'll have another go testing shortly.

            Show
            dobedobedoh Andrew Nicols added a comment - I've been trying to get around to testing this myself too for the past few days but haven't had a chance. The last time that I checked, it was all working correctly both with, and without X-SendFile. I also tried with nginx and X-Accel-Redirect just for completeness. I'll have another go testing shortly.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Yup - you can add a double-tick for me too. All works as expected wit Chrome on Mac OS. Tested with one commit reverted just to prove the issue too.

            Show
            dobedobedoh Andrew Nicols added a comment - Yup - you can add a double-tick for me too. All works as expected wit Chrome on Mac OS. Tested with one commit reverted just to prove the issue too.
            Hide
            matteo Matteo Scaramuccia added a comment -

            TNX Andrew!
            It seems we need to wait for replies from Dan/Ankit to identify the next step.

            Show
            matteo Matteo Scaramuccia added a comment - TNX Andrew! It seems we need to wait for replies from Dan/Ankit to identify the next step.
            Hide
            poltawski Dan Poltawski added a comment -

            Hi Matteo,

            I'm not going to have time to test this with x-sendfile in the near future (i'm running stock version of mac apache so need to compile it myself, and a bit pressed for time at the moment), but retesting without it and it seems it must've been cached or something and I am not seeing the problem when I tested. Originally I had not seen that Ankit was testing this with x-sendfile, which has lead to quite some confusion on this issue! Sorry about that.

            So, I think this is good to go and was ok in the first place. I can confirm that Ankit didn't have x-sendfile properly configured (just had the module enabled), so that is likely the cause of ankits problem.

            cheers,
            dan

            Show
            poltawski Dan Poltawski added a comment - Hi Matteo, I'm not going to have time to test this with x-sendfile in the near future (i'm running stock version of mac apache so need to compile it myself, and a bit pressed for time at the moment), but retesting without it and it seems it must've been cached or something and I am not seeing the problem when I tested. Originally I had not seen that Ankit was testing this with x-sendfile, which has lead to quite some confusion on this issue! Sorry about that. So, I think this is good to go and was ok in the first place. I can confirm that Ankit didn't have x-sendfile properly configured (just had the module enabled), so that is likely the cause of ankits problem. cheers, dan
            Hide
            matteo Matteo Scaramuccia added a comment -

            @Dan: TNX for your time and efforts spent for this issue too, appreciated !
            @Andrew: if you agree, could you close the peer review and send it to integration? TIA.

            Matteo

            Show
            matteo Matteo Scaramuccia added a comment - @Dan: TNX for your time and efforts spent for this issue too, appreciated ! @Andrew: if you agree, could you close the peer review and send it to integration? TIA. Matteo
            Hide
            poltawski Dan Poltawski added a comment -

            Thank you for your time!

            Show
            poltawski Dan Poltawski added a comment - Thank you for your time!
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Sending back for integration after much (more) testing

            Show
            dobedobedoh Andrew Nicols added a comment - Sending back for integration after much (more) testing
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Dan Poltawski FYI it was surprisingly easy to compile mod_xsendfile for Ma. The only issue I encountered is that httpd on OS X (Mountain Lion at least) uses a path to cc which doesn't exist on a normal install. I had to symlink /Applications/Xcode.app/Contents/Developer/Toolchains/OSX10.8.xctoolchain to /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/ before I could run the apxs -c command.

            Show
            dobedobedoh Andrew Nicols added a comment - Dan Poltawski FYI it was surprisingly easy to compile mod_xsendfile for Ma. The only issue I encountered is that httpd on OS X (Mountain Lion at least) uses a path to cc which doesn't exist on a normal install. I had to symlink /Applications/Xcode.app/Contents/Developer/Toolchains/OSX10.8.xctoolchain to /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/ before I could run the apxs -c command.
            Hide
            poltawski Dan Poltawski added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

            TIA and ciao

            Show
            poltawski Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
            Hide
            matteo Matteo Scaramuccia added a comment -

            Hi Dan,
            I'm in difficulty with git vs process.
            Your rebase request is part of the process (almost an automatic message) but in this issue things have been already integrated and then reverted (as per the discussion above) so rebasing can do nothing.

            Here two reverts (reverting the reverts ) are IMHO required to integrate the issue again:

            commit a151eb403eae727634050f58f389183eefca4630
            Author: Dan Poltawski <dan@moodle.com>
            Date:   Thu Jun 13 13:56:11 2013 +0800
             
                Revert "MDL-39832 Files: Revert MDL-39688 commit, f111746140fe80a821ae69e3bf518d950bf188a1"
             
                This reverts commit 3960c00e3cf129c60b01fecdfe2851ba9ef7ba59.
             
            commit db74804ff1245df4bdc0c8de187cfbe43a3ec0d7
            Author: Dan Poltawski <dan@moodle.com>
            Date:   Thu Jun 13 13:56:09 2013 +0800
             
                Revert "MDL-39832 Files: Fixed ETag format according with RFC2616."
             
                This reverts commit 9ab37ae3e07f54bc08644f8462d94bed68ff6c9b.
             
            commit 0a006517e5620f32e8a22fddcb43103992e5b134
            Merge: f4c824c 9ab37ae
            Author: Dan Poltawski <dan@moodle.com>
            Date:   Mon Jun 10 15:42:45 2013 +0800
             
                Merge branch 'm26_MDL-39832_Fix_Chrome_Issues_ETag_XSendfile' of https://github.com/scara/moodle
             
            commit 9ab37ae3e07f54bc08644f8462d94bed68ff6c9b
            Author: Matteo Scaramuccia <moodle@matteoscaramuccia.com>
            Date:   Sat Jun 1 12:34:40 2013 +0200
             
                MDL-39832 Files: Fixed ETag format according with RFC2616.
             
                ETag must be double quoted,
                http://tools.ietf.org/html/rfc2616#section-3.11:
             
                      entity-tag = [ weak ] opaque-tag
                      weak       = "W/"
                      opaque-tag = quoted-string
             
            commit 3960c00e3cf129c60b01fecdfe2851ba9ef7ba59
            Author: Matteo Scaramuccia <moodle@matteoscaramuccia.com>
            Date:   Sat Jun 1 12:21:29 2013 +0200
             
                MDL-39832 Files: Revert MDL-39688 commit, f111746140fe80a821ae69e3bf518d950bf188a1
             
                MDL-39688 has been successful in providing a solution for the issue but further
                investigations done in MDL-39832 have got the evidence that the given solution
                is more likely a workaround.

            I doubt if I need to push almost a new branch (e.g. m26_MDL-39832_Fix_Chrome_Issues_ETag_XSendfile_Reverting), to apply the correct reverting: any advice will be greatly appreciated.

            TIA,
            Matteo

            Show
            matteo Matteo Scaramuccia added a comment - Hi Dan, I'm in difficulty with git vs process. Your rebase request is part of the process (almost an automatic message) but in this issue things have been already integrated and then reverted (as per the discussion above) so rebasing can do nothing. Here two reverts (reverting the reverts ) are IMHO required to integrate the issue again: commit a151eb403eae727634050f58f389183eefca4630 Author: Dan Poltawski <dan@moodle.com> Date: Thu Jun 13 13:56:11 2013 +0800   Revert "MDL-39832 Files: Revert MDL-39688 commit, f111746140fe80a821ae69e3bf518d950bf188a1"   This reverts commit 3960c00e3cf129c60b01fecdfe2851ba9ef7ba59.   commit db74804ff1245df4bdc0c8de187cfbe43a3ec0d7 Author: Dan Poltawski <dan@moodle.com> Date: Thu Jun 13 13:56:09 2013 +0800   Revert "MDL-39832 Files: Fixed ETag format according with RFC2616."   This reverts commit 9ab37ae3e07f54bc08644f8462d94bed68ff6c9b.   commit 0a006517e5620f32e8a22fddcb43103992e5b134 Merge: f4c824c 9ab37ae Author: Dan Poltawski <dan@moodle.com> Date: Mon Jun 10 15:42:45 2013 +0800   Merge branch 'm26_MDL-39832_Fix_Chrome_Issues_ETag_XSendfile' of https://github.com/scara/moodle   commit 9ab37ae3e07f54bc08644f8462d94bed68ff6c9b Author: Matteo Scaramuccia <moodle@matteoscaramuccia.com> Date: Sat Jun 1 12:34:40 2013 +0200   MDL-39832 Files: Fixed ETag format according with RFC2616.   ETag must be double quoted, http://tools.ietf.org/html/rfc2616#section-3.11:   entity-tag = [ weak ] opaque-tag weak = "W/" opaque-tag = quoted-string   commit 3960c00e3cf129c60b01fecdfe2851ba9ef7ba59 Author: Matteo Scaramuccia <moodle@matteoscaramuccia.com> Date: Sat Jun 1 12:21:29 2013 +0200   MDL-39832 Files: Revert MDL-39688 commit, f111746140fe80a821ae69e3bf518d950bf188a1   MDL-39688 has been successful in providing a solution for the issue but further investigations done in MDL-39832 have got the evidence that the given solution is more likely a workaround. I doubt if I need to push almost a new branch (e.g. m26_ MDL-39832 _Fix_Chrome_Issues_ETag_XSendfile_Reverting ), to apply the correct reverting: any advice will be greatly appreciated. TIA, Matteo
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Hi Matteo,

            I think you should ignore the reverts performed before, just imagine they never happened. Just provide clean branches with the changes you want to be applied to every current upstream branch. I must confess I'm a bit disoriented about what is remaining to be integrated here, please clarify.

            Also, I'm keeping this on-hold for this week because practically everybody is out @ the AU Moot these days and it seems that the most important part of this issue is to test/verify nothing is borked. And there is already people experimented to test this that surely will perform the task far better than Sam or me (the only two available this week).

            Thanks for the big effort, ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Hi Matteo, I think you should ignore the reverts performed before, just imagine they never happened. Just provide clean branches with the changes you want to be applied to every current upstream branch. I must confess I'm a bit disoriented about what is remaining to be integrated here, please clarify. Also, I'm keeping this on-hold for this week because practically everybody is out @ the AU Moot these days and it seems that the most important part of this issue is to test/verify nothing is borked. And there is already people experimented to test this that surely will perform the task far better than Sam or me (the only two available this week). Thanks for the big effort, ciao
            Hide
            matteo Matteo Scaramuccia added a comment -

            Hi Eloy,
            taking 2.5 (m25_MDL-39832_Fix_Chrome_Issues_ETag_XSendfile) as an example, here are the two commits to be applied for this issue, MDL-39832:

            1. https://github.com/scara/moodle/commit/d618e3c07d36272809b84d72df51067f1daa51c7: reverting the work done by Petr in MDL-39688. It works but it removed the possibility to nicely cache, browser side, the Partial Responses (HTTP 206)
            2. https://github.com/scara/moodle/commit/52286ac358bbd81e8291cf0a85054ba63603732e: adding the missing double quotes as it has been identified to be the proper fix for MDL-39688.

            If you look at the 2.6 branch you'll see that the two commits have been already applied from my branch and then reverted due to the bad testing result, which has been found to be inconsistent in the next integration round.

            Shortly, for 2.3, 2.4, 2.5 you already have the branches (I'll rebase them) while for 2.6 we need to decide if:
            a. you revert the two reverts done by Dan w/ no need for rebasing on my side
            b. I re-create the two commits reverting Dan's reverts, in a new branch since my proposal for 2.6 has been already applied into master
            c. I apply the required fix, maybe not with the same commits history used in the other branches, in a new branch without explicitly reverting Dan's reverts

            The goal should IMHO be how to keep the history consistent between branches and I'm wondering if (a) could be the right option.

            Show
            matteo Matteo Scaramuccia added a comment - Hi Eloy, taking 2.5 (m25_ MDL-39832 _Fix_Chrome_Issues_ETag_XSendfile) as an example, here are the two commits to be applied for this issue, MDL-39832 : https://github.com/scara/moodle/commit/d618e3c07d36272809b84d72df51067f1daa51c7: reverting the work done by Petr in MDL-39688 . It works but it removed the possibility to nicely cache, browser side, the Partial Responses ( HTTP 206 ) https://github.com/scara/moodle/commit/52286ac358bbd81e8291cf0a85054ba63603732e: adding the missing double quotes as it has been identified to be the proper fix for MDL-39688 . If you look at the 2.6 branch you'll see that the two commits have been already applied from my branch and then reverted due to the bad testing result, which has been found to be inconsistent in the next integration round. Shortly, for 2.3, 2.4, 2.5 you already have the branches (I'll rebase them) while for 2.6 we need to decide if: a. you revert the two reverts done by Dan w/ no need for rebasing on my side b. I re-create the two commits reverting Dan's reverts, in a new branch since my proposal for 2.6 has been already applied into master c. I apply the required fix, maybe not with the same commits history used in the other branches, in a new branch without explicitly reverting Dan's reverts The goal should IMHO be how to keep the history consistent between branches and I'm wondering if (a) could be the right option.
            Show
            matteo Matteo Scaramuccia added a comment - - edited Hi Eloy, back again, I've made a mistake in my previous comment; all the branches have been already "fixed" and "reverted", e.g.: https://github.com/moodle/moodle/commits/master/lib/filelib.php https://github.com/moodle/moodle/commits/MOODLE_25_STABLE/lib/filelib.php https://github.com/moodle/moodle/commits/MOODLE_24_STABLE/lib/filelib.php https://github.com/moodle/moodle/commits/MOODLE_23_STABLE/lib/filelib.php
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            lol, I'm getting crazy with this, hahaha.

            I don't mind what has happened until now in any of the branches. If something was sent and later reverted it 100% the same than assuming it never landed in first term.

            So, right now, the master branch is into A status and you want to move it to B status, so all we need is a commit performing the changes from A to B.

            I really don't get the point about reverting the reverts not what has been partially or totally added modified by other issue. Just add as many commits you need to go successfully to B from current status. if for master you need 2 or more commits, one undoing what was done in another issue and another for the fixes originally related to this issue, np here at all.

            But I think we must not be introducing re-re-re-reverts. Just imagine it never happened, really.

            If that is alternative (c) in your list above... then go for it, IMO. Erase those 2 commits from your mind (the original and its revert, they sum, together, 0 changes).

            Not sure if I'm missing something or no... sorry, thanks and ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - lol, I'm getting crazy with this, hahaha. I don't mind what has happened until now in any of the branches. If something was sent and later reverted it 100% the same than assuming it never landed in first term. So, right now, the master branch is into A status and you want to move it to B status, so all we need is a commit performing the changes from A to B. I really don't get the point about reverting the reverts not what has been partially or totally added modified by other issue. Just add as many commits you need to go successfully to B from current status. if for master you need 2 or more commits, one undoing what was done in another issue and another for the fixes originally related to this issue, np here at all. But I think we must not be introducing re-re-re-reverts. Just imagine it never happened, really. If that is alternative (c) in your list above... then go for it, IMO. Erase those 2 commits from your mind (the original and its revert, they sum, together, 0 changes). Not sure if I'm missing something or no... sorry, thanks and ciao
            Hide
            matteo Matteo Scaramuccia added a comment -

            OK,
            will do all the required changes to move from now (A) to what required (B).

            Matteo

            Show
            matteo Matteo Scaramuccia added a comment - OK, will do all the required changes to move from now (A) to what required (B). Matteo
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Many thanks Matteo!

            I'm sending this back to development, so will be picked next week (or when available, no pressure!).

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Many thanks Matteo! I'm sending this back to development, so will be picked next week (or when available, no pressure!). Ciao
            Hide
            cibot CiBoT added a comment -

            Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            matteo Matteo Scaramuccia added a comment -
            Show
            matteo Matteo Scaramuccia added a comment - For the/my record, interesting reading: https://www.kernel.org/pub/software/scm/git/docs/howto/revert-a-faulty-merge.txt
            Hide
            matteo Matteo Scaramuccia added a comment -

            Hi Andrew,
            would you be so kind and patient to perform another review and propose the fix back again to the integration review?

            TIA!
            Matteo

            Show
            matteo Matteo Scaramuccia added a comment - Hi Andrew, would you be so kind and patient to perform another review and propose the fix back again to the integration review? TIA! Matteo
            Hide
            poltawski Dan Poltawski added a comment -

            (Sorry I didn't see this message earlier Matteo).

            Basically, you need to re-apply the commits, which is kind of ugly I agree - and rebase doesn't do the right thing, because it thinks its already been commited.

            Show
            poltawski Dan Poltawski added a comment - (Sorry I didn't see this message earlier Matteo). Basically, you need to re-apply the commits, which is kind of ugly I agree - and rebase doesn't do the right thing, because it thinks its already been commited.
            Hide
            matteo Matteo Scaramuccia added a comment -

            NP Dan!
            Already done and learned something new which is fine .

            Show
            matteo Matteo Scaramuccia added a comment - NP Dan! Already done and learned something new which is fine .
            Hide
            matteo Matteo Scaramuccia added a comment -

            Rebased, just in case it will be integrated the next week .

            Show
            matteo Matteo Scaramuccia added a comment - Rebased, just in case it will be integrated the next week .
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Thanks Matteo, those commits look pretty clear now!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Thanks Matteo, those commits look pretty clear now!
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Peer review looks good to me still (no change). Submitting for integration now.

            Show
            dobedobedoh Andrew Nicols added a comment - Peer review looks good to me still (no change). Submitting for integration now.
            Hide
            matteo Matteo Scaramuccia added a comment -

            Rebased.

            Show
            matteo Matteo Scaramuccia added a comment - Rebased.
            Hide
            marina Marina Glancy added a comment -

            Thanks guys, this has been integrated in 2.3, 2.4, 2.5 and master.
            Dan volunteered to be a tester

            Show
            marina Marina Glancy added a comment - Thanks guys, this has been integrated in 2.3, 2.4, 2.5 and master. Dan volunteered to be a tester
            Hide
            poltawski Dan Poltawski added a comment -

            Hurray, passed. Thanks Matteo.

            I noticed a few things while testing:

            1. The quicktime plugin seems screwed on my machine on first load. It seems to work ok one cached. This was unrelated to this patch and also to xsendfile on or off. I think this was the problem I saw when testing earlier.
            2. We seem to be serving a YUI file mistakenly using x-sendfile:

              (20023)The given path was above the root path: xsendfile: unable to find file: /Users/danp/git/integration/lib/yuilib/3.9.1/build/assets/skins/sam/sprite_icons.png, referer: https://dan.moodle.local/integration/mod/forum/post.php?forum=1
              

              However, I couldn't reproduce this when I tried agian.

            Show
            poltawski Dan Poltawski added a comment - Hurray, passed. Thanks Matteo. I noticed a few things while testing: The quicktime plugin seems screwed on my machine on first load. It seems to work ok one cached. This was unrelated to this patch and also to xsendfile on or off. I think this was the problem I saw when testing earlier. We seem to be serving a YUI file mistakenly using x-sendfile: (20023)The given path was above the root path: xsendfile: unable to find file: /Users/danp/git/integration/lib/yuilib/3.9.1/build/assets/skins/sam/sprite_icons.png, referer: https://dan.moodle.local/integration/mod/forum/post.php?forum=1 However, I couldn't reproduce this when I tried agian.
            Hide
            poltawski Dan Poltawski added a comment -

            ps. Thanks Andrew for the xsendfile note. It was suprisingly easy and that symlink tip helped me avoid messing around to fix it.

            Show
            poltawski Dan Poltawski added a comment - ps. Thanks Andrew for the xsendfile note. It was suprisingly easy and that symlink tip helped me avoid messing around to fix it.
            Hide
            matteo Matteo Scaramuccia added a comment -

            Regarding with the xsendfile error, let me share some of my notes: XSendfilePath should be defined e.g. as /Users/danp/git, without the trailing slash and using forward slashes for both *nix and Windows... if everything worked as expected there was no issue with AllowOverride FileInfo too and I've no clear reasons for that error in your log file.

            Show
            matteo Matteo Scaramuccia added a comment - Regarding with the xsendfile error, let me share some of my notes: XSendfilePath should be defined e.g. as /Users/danp/git , without the trailing slash and using forward slashes for both *nix and Windows... if everything worked as expected there was no issue with AllowOverride FileInfo too and I've no clear reasons for that error in your log file.
            Hide
            poltawski Dan Poltawski added a comment -

            Surely not - are we not restricting xsendfile served files to moodledata? I didn't think it was a solution for serving dirroot files.

            Show
            poltawski Dan Poltawski added a comment - Surely not - are we not restricting xsendfile served files to moodledata? I didn't think it was a solution for serving dirroot files.
            Hide
            matteo Matteo Scaramuccia added a comment -

            Good point!
            This morning I didn't explicitly focus on being an error related to what should be supposed to be served as a static file, being under dirroot because I'm supposing this behaviour to be by-design: those IMG files are referenced in CSS files as relative to the CSS themselves, the CSS files being served through a PHP helper - I know you know it , just for the record - for managing also the theme versioning and performing other required stuff like rewriting the path of the imgs too (ref.: yui_compo.php) i.e. that is the reason why X-Sendfile is running even on dirroot, raw details: theme/yui_image.php.

            Show
            matteo Matteo Scaramuccia added a comment - Good point! This morning I didn't explicitly focus on being an error related to what should be supposed to be served as a static file, being under dirroot because I'm supposing this behaviour to be by-design: those IMG files are referenced in CSS files as relative to the CSS themselves, the CSS files being served through a PHP helper - I know you know it , just for the record - for managing also the theme versioning and performing other required stuff like rewriting the path of the imgs too (ref.: yui_compo.php ) i.e. that is the reason why X-Sendfile is running even on dirroot , raw details: theme/yui_image.php .
            Hide
            damyon Damyon Wiese added a comment -

            a single bug fix
            a drop in a waterfall
            hear the mighty roar

            Thanks for your contribution!

            Show
            damyon Damyon Wiese added a comment - a single bug fix a drop in a waterfall hear the mighty roar Thanks for your contribution!
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -
            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Just to comment that his causes a regression on etag verifications that are being commented @ https://tracker.moodle.org/browse/MDL-38743?focusedCommentId=250106&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-250106 Ciao
            Hide
            kabalin Ruslan Kabalin added a comment -

            We have encountered the problem with ETags recently, so I will write here in case someone find it useful. Basically, about month ago all of a sudden Chrome stopped playing videos on Moodle (presumably after some Chrome or Nginx update, as there were no configuration changes or anything that could affect it). We are using Nginx on the frontends and $CFG->xsendfile settings to make Nginx deliver static data directly from Moodle datastore. The video is normally retrieved using byte-range requests, which Nginx handled properly.

            Clearing the cache in Chrome helped and made video work, but only once, the next page refresh or visiting page containing the video the second time resulted in video error. The Nginx we were using is 1.2 on Debian Wheezy. Investigation revealed that disabling ETags header propogation in Nginx for Moodle data static content solved the issue (it used add_header Etag $upstream_http_etag; to propagate ETag coming from Moodle response on background to the static file output it generate), all video started working fine and all partial requests were handled properly.

            Further investigation and tests revealed that upgrading Nginx to 1.6 (from wheezy-backports) solves the problem even with the same ETags configuration in Nginx - without any changes to anything, it just started working fine in Chrome with newer version of Nginx. An interesting thing I spotted: if I remove ETag coming from Moodle, Nginx will now generate own "strong" ETag by default, and it will be the same for all parts of the same file, no matter which range is requested, so this confirms the earlier discussion that same ETag is used for all parts of the same file as per RFC. Otherwise, there are no differences in headers, so the issue is somewhat mysterious. I am going to report the bug to Nginx maintainers in Debian.

            Show
            kabalin Ruslan Kabalin added a comment - We have encountered the problem with ETags recently, so I will write here in case someone find it useful. Basically, about month ago all of a sudden Chrome stopped playing videos on Moodle (presumably after some Chrome or Nginx update, as there were no configuration changes or anything that could affect it). We are using Nginx on the frontends and $CFG->xsendfile settings to make Nginx deliver static data directly from Moodle datastore. The video is normally retrieved using byte-range requests, which Nginx handled properly. Clearing the cache in Chrome helped and made video work, but only once, the next page refresh or visiting page containing the video the second time resulted in video error. The Nginx we were using is 1.2 on Debian Wheezy. Investigation revealed that disabling ETags header propogation in Nginx for Moodle data static content solved the issue (it used add_header Etag $upstream_http_etag; to propagate ETag coming from Moodle response on background to the static file output it generate), all video started working fine and all partial requests were handled properly. Further investigation and tests revealed that upgrading Nginx to 1.6 (from wheezy-backports) solves the problem even with the same ETags configuration in Nginx - without any changes to anything, it just started working fine in Chrome with newer version of Nginx. An interesting thing I spotted: if I remove ETag coming from Moodle, Nginx will now generate own "strong" ETag by default, and it will be the same for all parts of the same file, no matter which range is requested, so this confirms the earlier discussion that same ETag is used for all parts of the same file as per RFC. Otherwise, there are no differences in headers, so the issue is somewhat mysterious. I am going to report the bug to Nginx maintainers in Debian.
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks for the detailed info Ruslan - have you seen the linked bugs to this issue? MDL-40002

            Show
            poltawski Dan Poltawski added a comment - Thanks for the detailed info Ruslan - have you seen the linked bugs to this issue? MDL-40002
            Hide
            kabalin Ruslan Kabalin added a comment -

            Dan Poltawski Yep I have seen them, but the change has been integrated a while ago, and there were no problems a long after till something changed within last two month.

            Show
            kabalin Ruslan Kabalin added a comment - Dan Poltawski Yep I have seen them, but the change has been integrated a while ago, and there were no problems a long after till something changed within last two month.

              People

              • Votes:
                0 Vote for this issue
                Watchers:
                6 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  9/Sep/13