Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0, 2.2.11, 2.3
    • Fix Version/s: 2.3
    • Component/s: Files API
    • Labels:
      None
    • Testing Instructions:
      Hide

      ETag testing - upload file to forum

      1/ diagnose http headers sent with the image - 304 expected on subsequent page loads (previously it was sending the whole file repeatedly)

      2/ verify that forum attachments are force downloaded, they should never open directly in browser

      X-Sendfile testing
      1/ install X-Sendile extension into apache (https://tn123.org/mod_xsendfile/)
      2/ enable X-Sendfile and whitelist dataroot directory
      3/ verify you can view forum attachments, download grades in XML format and bulk export users to Excel

      4/ Turn off X-Sendfile

      5/ verify you can view forum attachments, download grades in XML format and bulk export users to Excel

      6/ try some SCORM packages with and without X-Sendfile enabled

      Show
      ETag testing - upload file to forum 1/ diagnose http headers sent with the image - 304 expected on subsequent page loads (previously it was sending the whole file repeatedly) 2/ verify that forum attachments are force downloaded, they should never open directly in browser — X-Sendfile testing 1/ install X-Sendile extension into apache ( https://tn123.org/mod_xsendfile/ ) 2/ enable X-Sendfile and whitelist dataroot directory 3/ verify you can view forum attachments, download grades in XML format and bulk export users to Excel — 4/ Turn off X-Sendfile 5/ verify you can view forum attachments, download grades in XML format and bulk export users to Excel 6/ try some SCORM packages with and without X-Sendfile enabled
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      w18_MDL-26028_m23_xsendfile

      Description

      Add support for X-Sendfile. Related forum discussion: http://moodle.org/mod/forum/discuss.php?d=165470

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Tomasz Muras added a comment -

            The code is on github: https://github.com/enovation/moodle/tree/MDL-26028 . One important bit is that I had to change:
            protected function get_content_file_location()

            to:
            public function get_content_file_location()

            Show
            Tomasz Muras added a comment - The code is on github: https://github.com/enovation/moodle/tree/MDL-26028 . One important bit is that I had to change: protected function get_content_file_location() to: public function get_content_file_location()
            Hide
            Petr Skoda added a comment -

            Hello,
            did you verify that the headers returned through X-Sendfile are 100% the same compared to the current way (preferably on all types of servers, not just apache)? If not it might create unexpected XSS issues.

            I think that the benefit will be quite low because the biggest cost is our session, security checks and general overhead.

            In any case thanks a lot for evaluating this potential performance improvement! I am going to make this a subtask in the existing meta issue.

            Petr

            Show
            Petr Skoda added a comment - Hello, did you verify that the headers returned through X-Sendfile are 100% the same compared to the current way (preferably on all types of servers, not just apache)? If not it might create unexpected XSS issues. I think that the benefit will be quite low because the biggest cost is our session, security checks and general overhead. In any case thanks a lot for evaluating this potential performance improvement! I am going to make this a subtask in the existing meta issue. Petr
            Hide
            Hubert Chathi added a comment -

            Lighttpd 1.4 uses the "X-LIGHTTPD-send-file" header for this. I'll try this out later on (today hopefully).

            FWIW, the headers might not be 100% the same – the web server may calculate its own ETag header (and maybe others).

            Show
            Hubert Chathi added a comment - Lighttpd 1.4 uses the "X-LIGHTTPD-send-file" header for this. I'll try this out later on (today hopefully). FWIW, the headers might not be 100% the same – the web server may calculate its own ETag header (and maybe others).
            Hide
            Petr Skoda added a comment -

            The files uploaded by students have intentionally borked file headers (aka forcedownload), if anything fixes them we have a XSS problem.

            Show
            Petr Skoda added a comment - The files uploaded by students have intentionally borked file headers (aka forcedownload), if anything fixes them we have a XSS problem.
            Hide
            Hubert Chathi added a comment - - edited

            Trying with lighttpd, downloading a file from the front page (so all users have access), the headers are the same except for the Accept-Ranges: header – lighttpd sends Accept-Ranges: none, while plain Moodle sends Accept-Ranges: bytes. I'm going to do some more testing.

            My updates (lighttpd support, added language strings, add support to the send_file() function, and some cleanups) are available here: http://vcs.uhoreg.ca/git/cgit/moodle/log/?h=MDL-26028

            Nginx's X-Accel-Redirect doesn't seem very straight forward, and I think we should get someone who uses Nginx to test that setting out (and document how Nginx needs to be configured for it to work) before we add that option.

            Edit: updated link to my git browser

            Show
            Hubert Chathi added a comment - - edited Trying with lighttpd, downloading a file from the front page (so all users have access), the headers are the same except for the Accept-Ranges: header – lighttpd sends Accept-Ranges: none, while plain Moodle sends Accept-Ranges: bytes. I'm going to do some more testing. My updates (lighttpd support, added language strings, add support to the send_file() function, and some cleanups) are available here: http://vcs.uhoreg.ca/git/cgit/moodle/log/?h=MDL-26028 Nginx's X-Accel-Redirect doesn't seem very straight forward, and I think we should get someone who uses Nginx to test that setting out (and document how Nginx needs to be configured for it to work) before we add that option. Edit: updated link to my git browser
            Hide
            Hubert Chathi added a comment -

            The files uploaded by students have intentionally borked file headers (aka forcedownload), if anything fixes them we have a XSS problem.

            For lighttpd the docs (well, blog post) says: "All the response headers from the backend are forwarded, just Content-Length is added.". And when I tested it, it maintained the "Content-Type: application/x-forcedownload" and "Content-Disposition: attachment; ..." headers.

            From what I can tell, the Apache module should maintain those headers too (it looks like it just modifies ETag, and drops Content-Encoding), but I don't have Apache to try it out.

            Show
            Hubert Chathi added a comment - The files uploaded by students have intentionally borked file headers (aka forcedownload), if anything fixes them we have a XSS problem. For lighttpd the docs (well, blog post) says: "All the response headers from the backend are forwarded, just Content-Length is added.". And when I tested it, it maintained the "Content-Type: application/x-forcedownload" and "Content-Disposition: attachment; ..." headers. From what I can tell, the Apache module should maintain those headers too (it looks like it just modifies ETag, and drops Content-Encoding), but I don't have Apache to try it out.
            Hide
            Hubert Chathi added a comment -

            I did some informal benchmarking on lighttpd 1.4 using ApacheBench, and here are the results for 'ab -n 1000 -c 100 "http://localhost/xsendfile/pluginfile.php/14/mod_resource/content/1/elisdoc.tar.gz?forcedownload=1"' (1000 requests 100 concurrent, downloading a file uploaded to the front page):

            Using X-Sendfile:

             
            This is ApacheBench, Version 2.3 <$Revision: 655654 $>
            Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
            Licensed to The Apache Software Foundation, http://www.apache.org/
             
            Benchmarking localhost (be patient)
            Completed 100 requests
            Completed 200 requests
            Completed 300 requests
            Completed 400 requests
            Completed 500 requests
            Completed 600 requests
            Completed 700 requests
            Completed 800 requests
            Completed 900 requests
            Completed 1000 requests
            Finished 1000 requests
             
             
            Server Software:        lighttpd/1.4.28
            Server Hostname:        localhost
            Server Port:            80
             
            Document Path:          /xsendfile/pluginfile.php/14/mod_resource/content/1/elisdoc.tar.gz?forcedownload=1
            Document Length:        163284427 bytes
             
            Concurrency Level:      100
            Time taken for tests:   188.595 seconds
            Complete requests:      1000
            Failed requests:        0
            Write errors:           0
            Total transferred:      163284902000 bytes
            HTML transferred:       163284427000 bytes
            Requests per second:    5.30 [#/sec] (mean)
            Time per request:       18859.505 [ms] (mean)
            Time per request:       188.595 [ms] (mean, across all concurrent requests)
            Transfer rate:          845504.23 [Kbytes/sec] received
             
            Connection Times (ms)
                          min  mean[+/-sd] median   max
            Connect:        0    2   1.5      2       8
            Processing:  7261 18413 4044.9  17673   34347
            Waiting:      266 4177 6043.5    525   19613
            Total:       7263 18415 4044.9  17675   34350
             
            Percentage of the requests served within a certain time (ms)
              50%  17675
              66%  17937
              75%  18456
              80%  20192
              90%  23707
              95%  26304
              98%  30332
              99%  31565
             100%  34350 (longest request)
            

            Without X-Sendfile:

             
            This is ApacheBench, Version 2.3 <$Revision: 655654 $>
            Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
            Licensed to The Apache Software Foundation, http://www.apache.org/
             
            Benchmarking localhost (be patient)
            Completed 100 requests
            Completed 200 requests
            Completed 300 requests
            Completed 400 requests
            Completed 500 requests
            Completed 600 requests
            Completed 700 requests
            Completed 800 requests
            Completed 900 requests
            Completed 1000 requests
            Finished 1000 requests
             
             
            Server Software:        lighttpd/1.4.28
            Server Hostname:        localhost
            Server Port:            80
             
            Document Path:          /xsendfile/pluginfile.php/14/mod_resource/content/1/elisdoc.tar.gz?forcedownload=1
            Document Length:        163284427 bytes
             
            Concurrency Level:      100
            Time taken for tests:   321.726 seconds
            Complete requests:      1000
            Failed requests:        0
            Write errors:           0
            Total transferred:      163284903000 bytes
            HTML transferred:       163284427000 bytes
            Requests per second:    3.11 [#/sec] (mean)
            Time per request:       32172.641 [ms] (mean)
            Time per request:       321.726 [ms] (mean, across all concurrent requests)
            Transfer rate:          495632.03 [Kbytes/sec] received
             
            Connection Times (ms)
                          min  mean[+/-sd] median   max
            Connect:        0    0   0.8      0       4
            Processing:  3171 30678 5574.6  31112   40680
            Waiting:      462 29776 5608.2  30196   39994
            Total:       3175 30679 5574.0  31113   40680
             
            Percentage of the requests served within a certain time (ms)
              50%  31113
              66%  31372
              75%  31605
              80%  31717
              90%  37622
              95%  39641
              98%  40227
              99%  40358
             100%  40680 (longest request)
            

            Also, when using X-Sendfile, the lighttpd process was using about 20% CPU according to top (vs around 80% CPU without X-Sendfile), and the PHP processes stayed at their normal ~80-90MB memory usage (vs jumping up to ~100-200MB without X-Sendfile)

            Show
            Hubert Chathi added a comment - I did some informal benchmarking on lighttpd 1.4 using ApacheBench, and here are the results for 'ab -n 1000 -c 100 "http://localhost/xsendfile/pluginfile.php/14/mod_resource/content/1/elisdoc.tar.gz?forcedownload=1"' (1000 requests 100 concurrent, downloading a file uploaded to the front page): Using X-Sendfile:   This is ApacheBench, Version 2.3 <$Revision: 655654 $> Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/ Licensed to The Apache Software Foundation, http://www.apache.org/   Benchmarking localhost (be patient) Completed 100 requests Completed 200 requests Completed 300 requests Completed 400 requests Completed 500 requests Completed 600 requests Completed 700 requests Completed 800 requests Completed 900 requests Completed 1000 requests Finished 1000 requests     Server Software: lighttpd/1.4.28 Server Hostname: localhost Server Port: 80   Document Path: /xsendfile/pluginfile.php/14/mod_resource/content/1/elisdoc.tar.gz?forcedownload=1 Document Length: 163284427 bytes   Concurrency Level: 100 Time taken for tests: 188.595 seconds Complete requests: 1000 Failed requests: 0 Write errors: 0 Total transferred: 163284902000 bytes HTML transferred: 163284427000 bytes Requests per second: 5.30 [#/sec] (mean) Time per request: 18859.505 [ms] (mean) Time per request: 188.595 [ms] (mean, across all concurrent requests) Transfer rate: 845504.23 [Kbytes/sec] received   Connection Times (ms) min mean[+/-sd] median max Connect: 0 2 1.5 2 8 Processing: 7261 18413 4044.9 17673 34347 Waiting: 266 4177 6043.5 525 19613 Total: 7263 18415 4044.9 17675 34350   Percentage of the requests served within a certain time (ms) 50% 17675 66% 17937 75% 18456 80% 20192 90% 23707 95% 26304 98% 30332 99% 31565 100% 34350 (longest request) Without X-Sendfile:   This is ApacheBench, Version 2.3 <$Revision: 655654 $> Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/ Licensed to The Apache Software Foundation, http://www.apache.org/   Benchmarking localhost (be patient) Completed 100 requests Completed 200 requests Completed 300 requests Completed 400 requests Completed 500 requests Completed 600 requests Completed 700 requests Completed 800 requests Completed 900 requests Completed 1000 requests Finished 1000 requests     Server Software: lighttpd/1.4.28 Server Hostname: localhost Server Port: 80   Document Path: /xsendfile/pluginfile.php/14/mod_resource/content/1/elisdoc.tar.gz?forcedownload=1 Document Length: 163284427 bytes   Concurrency Level: 100 Time taken for tests: 321.726 seconds Complete requests: 1000 Failed requests: 0 Write errors: 0 Total transferred: 163284903000 bytes HTML transferred: 163284427000 bytes Requests per second: 3.11 [#/sec] (mean) Time per request: 32172.641 [ms] (mean) Time per request: 321.726 [ms] (mean, across all concurrent requests) Transfer rate: 495632.03 [Kbytes/sec] received   Connection Times (ms) min mean[+/-sd] median max Connect: 0 0 0.8 0 4 Processing: 3171 30678 5574.6 31112 40680 Waiting: 462 29776 5608.2 30196 39994 Total: 3175 30679 5574.0 31113 40680   Percentage of the requests served within a certain time (ms) 50% 31113 66% 31372 75% 31605 80% 31717 90% 37622 95% 39641 98% 40227 99% 40358 100% 40680 (longest request) Also, when using X-Sendfile, the lighttpd process was using about 20% CPU according to top (vs around 80% CPU without X-Sendfile), and the PHP processes stayed at their normal ~80-90MB memory usage (vs jumping up to ~100-200MB without X-Sendfile)
            Hide
            Gabriel Mazetto added a comment -

            We have conduced a benchmark to test how this could improove overall download performance and the results are attached

            Show
            Gabriel Mazetto added a comment - We have conduced a benchmark to test how this could improove overall download performance and the results are attached
            Hide
            Petr Skoda added a comment -

            Thanks for the benchmarks, did you try to evaluate the results somehow?

            Show
            Petr Skoda added a comment - Thanks for the benchmarks, did you try to evaluate the results somehow?
            Hide
            Gabriel Mazetto added a comment -

            Here is the code used to run the benchmark.

            For the "optimizations" we used:

            EnableMMAP On
            EnableSendfile On

            On apache Vhost

            And for the "mod_xsendfile" modified file.php we did modified file.php near:

            // ========================================
            // finally send the file
            // ========================================
            session_write_close(); // unlock session during fileserving
            $filename = $args[count($args)-1];
            header('X-Sendfile: '.$pathname);
            die();
            send_file($pathname, $filename, $lifetime, $CFG->filteruploadedfiles, false, $forcedownload);

            Show
            Gabriel Mazetto added a comment - Here is the code used to run the benchmark. For the "optimizations" we used: EnableMMAP On EnableSendfile On On apache Vhost And for the "mod_xsendfile" modified file.php we did modified file.php near: // ======================================== // finally send the file // ======================================== session_write_close(); // unlock session during fileserving $filename = $args [count($args)-1] ; header('X-Sendfile: '.$pathname); die(); send_file($pathname, $filename, $lifetime, $CFG->filteruploadedfiles, false, $forcedownload);
            Hide
            Tomasz Muras added a comment -

            The difference between Hubert's and Gabriel's tests and mine is that I was downloading a file that has required user to be authorized - I made ab to pass a cookie to authenticate. In such a case the speed-up may be smaller as there is more PHP logic executed before the file starts downloading.

            Anyway, it looks to me that it makes sense to add XSendfile support - the only problem I have is testing with other web servers than Apache.

            Show
            Tomasz Muras added a comment - The difference between Hubert's and Gabriel's tests and mine is that I was downloading a file that has required user to be authorized - I made ab to pass a cookie to authenticate. In such a case the speed-up may be smaller as there is more PHP logic executed before the file starts downloading. Anyway, it looks to me that it makes sense to add XSendfile support - the only problem I have is testing with other web servers than Apache.
            Hide
            Gabriel Mazetto added a comment -

            Petr Škoda, we found that, for small files, file.php will performs better (on response time), but as the filesize grows X-Sendfile with optimizations starts to became very similar, and with files bigger then 10Mb it performs better (I have no idea why 100Mb benchmark stated it wrong but I can guess there was a problem collecting the data near that test).

            This is some kind obvious, as it's faster, for small files, to just let the php handle it in one step, not two (like what is needed with X-Sendfile).

            But the chief gain is that X-Sendfile can handle more "Hits per second" or better saying it can deliver more files at the same time then "php" version.

            This can be seen by looking at "duration" graphs. (Fewer times are better)

            Show
            Gabriel Mazetto added a comment - Petr Škoda, we found that, for small files, file.php will performs better (on response time), but as the filesize grows X-Sendfile with optimizations starts to became very similar, and with files bigger then 10Mb it performs better (I have no idea why 100Mb benchmark stated it wrong but I can guess there was a problem collecting the data near that test). This is some kind obvious, as it's faster, for small files, to just let the php handle it in one step, not two (like what is needed with X-Sendfile). But the chief gain is that X-Sendfile can handle more "Hits per second" or better saying it can deliver more files at the same time then "php" version. This can be seen by looking at "duration" graphs. (Fewer times are better)
            Hide
            Hubert Chathi added a comment -

            I can test with lighttpd. The other servers that I know of that support this are Nginx and Cherokee. (I may try out Cherokee at some point.) I don't know if IIS has any similar support (whether official or not). As far as what to test, we need to test that it serves the correct file contents (obviously), and that it doesn't munge important headers (especially the headers related to forcedownload). Is there anything else?

            Show
            Hubert Chathi added a comment - I can test with lighttpd. The other servers that I know of that support this are Nginx and Cherokee. (I may try out Cherokee at some point.) I don't know if IIS has any similar support (whether official or not). As far as what to test, we need to test that it serves the correct file contents (obviously), and that it doesn't munge important headers (especially the headers related to forcedownload). Is there anything else?
            Hide
            Alexey Dushechkin added a comment -

            Major benefit from sendfile support is ability to offload file sending (which may take significant amount of time because of large file size/slow client connection) to lightweight threaded webservers (lighttpd, nginx) instead of heavy php processes. It is very inefficient to use full-blown php process to simply serve a static file.

            Currently I am investigating nginx + php-fpm setup for Moodle, and definitely can help with testing of this feature.

            Show
            Alexey Dushechkin added a comment - Major benefit from sendfile support is ability to offload file sending (which may take significant amount of time because of large file size/slow client connection) to lightweight threaded webservers (lighttpd, nginx) instead of heavy php processes. It is very inefficient to use full-blown php process to simply serve a static file. Currently I am investigating nginx + php-fpm setup for Moodle, and definitely can help with testing of this feature.
            Hide
            Gabriel Mazetto added a comment -

            Just came back to this ticket to see if it's already accepted or not, and as Alexey said, if we consider nginx + php-fpm, apache2 + php-fpm, or even apache2 + php with fastcgi, offloading file transmission to the webserver means that your PHP process is free to serve other users while the webserver takes the responsability to sending the file.

            Just imagine these two scenarios:

            • A not so big file but user with really slow connection
            • A big file (maybe a movie) and a user with a quite decent connection

            In both situations, the PHP process will stay "blocked" throttling the download and not serving the other users. Just imagine that you spawned 10 process and you have 10 users downloading files with a really slow connection, your site became irresponsive until the users finish their downloads.

            X-Sendfile is a major improvement and should really be considered as a major feature for next release.

            If no one volunteers to code this feature, I will probably do it, but will need some help to not mess existing features.

            Show
            Gabriel Mazetto added a comment - Just came back to this ticket to see if it's already accepted or not, and as Alexey said, if we consider nginx + php-fpm, apache2 + php-fpm, or even apache2 + php with fastcgi, offloading file transmission to the webserver means that your PHP process is free to serve other users while the webserver takes the responsability to sending the file. Just imagine these two scenarios: A not so big file but user with really slow connection A big file (maybe a movie) and a user with a quite decent connection In both situations, the PHP process will stay "blocked" throttling the download and not serving the other users. Just imagine that you spawned 10 process and you have 10 users downloading files with a really slow connection, your site became irresponsive until the users finish their downloads. X-Sendfile is a major improvement and should really be considered as a major feature for next release. If no one volunteers to code this feature, I will probably do it, but will need some help to not mess existing features.
            Hide
            Hubert Chathi added a comment -

            Gabriel: it has already been coded. See http://vcs.uhoreg.ca/git/cgit/moodle/log/?h=MDL-26028. It just needs to be tested to make sure that:

            • all file downloads are handled
            • downloads are processed correctly (sends the correct file contents)
            • Petr's security concerns are handled correctly

            I've lightly tested it in lighttpd, and for the things that I've tested, the security things were fine, but it needs to be tested more (and it needs to be documented what has been tested an how), and on different servers.

            Show
            Hubert Chathi added a comment - Gabriel: it has already been coded. See http://vcs.uhoreg.ca/git/cgit/moodle/log/?h=MDL-26028 . It just needs to be tested to make sure that: all file downloads are handled downloads are processed correctly (sends the correct file contents) Petr's security concerns are handled correctly I've lightly tested it in lighttpd, and for the things that I've tested, the security things were fine, but it needs to be tested more (and it needs to be documented what has been tested an how), and on different servers.
            Hide
            Gabriel Mazetto added a comment -

            Well thank you for that, I didn't saw your first comment with the link to the repository.
            I will make a test with nginx and paste configuration instructions.

            Show
            Gabriel Mazetto added a comment - Well thank you for that, I didn't saw your first comment with the link to the repository. I will make a test with nginx and paste configuration instructions.
            Hide
            Hubert Chathi added a comment - - edited

            Ah, good, nginx. I'm not sure that I completely understood its X-Accel-Redirect documentation, so it's good if you can test it (and fix it if it's incorrect).

            Show
            Hubert Chathi added a comment - - edited Ah, good, nginx. I'm not sure that I completely understood its X-Accel-Redirect documentation, so it's good if you can test it (and fix it if it's incorrect).
            Hide
            Gabriel Mazetto added a comment - - edited

            according to documentation:

            Note that the following HTTP headers aren't modified by nginx:

            Content-Type
            Content-Disposition
            Accept-Ranges
            Set-Cookie
            Cache-Control
            Expires

            source: http://wiki.nginx.org/XSendfile

            I also found it difficult to apply your modification as your code is based on January 2011 codebase. Could you please update it to a more recent version? If possible also could you please do it on a public accessible git address (like on github)?

            Show
            Gabriel Mazetto added a comment - - edited according to documentation: Note that the following HTTP headers aren't modified by nginx: Content-Type Content-Disposition Accept-Ranges Set-Cookie Cache-Control Expires source: http://wiki.nginx.org/XSendfile I also found it difficult to apply your modification as your code is based on January 2011 codebase. Could you please update it to a more recent version? If possible also could you please do it on a public accessible git address (like on github)?
            Hide
            Hubert Chathi added a comment - - edited

            Sorry, I don't have time to update the patch right away. I'll see if I have time soon. (By the way, in case you didn't notice, there are two patches that need to be applied – one by Tomasz, and one by myself.)

            It is on a public git repository; you can "git clone http://vcs.uhoreg.ca/git/moodle.git", and use the MDL-26028. If you have a recent git, you can just do "git clone -b MDL-26028 http://vcs.uhoreg.ca/git/moodle.git".

            Show
            Hubert Chathi added a comment - - edited Sorry, I don't have time to update the patch right away. I'll see if I have time soon. (By the way, in case you didn't notice, there are two patches that need to be applied – one by Tomasz, and one by myself.) It is on a public git repository; you can "git clone http://vcs.uhoreg.ca/git/moodle.git ", and use the MDL-26028 . If you have a recent git, you can just do "git clone -b MDL-26028 http://vcs.uhoreg.ca/git/moodle.git ".
            Hide
            Loic Jeannin added a comment -

            I've implemented a more "spread" use of nginx's X-Accel-Redirect against v2.2.2. You can find my initial work here
            http://http://git.digitalsk.com.br/moodle.git branch xaccel

            To make it work you will have to configure 2 internal locations in nginx, one for your root dir and one for moodleadta:
            — snip —
            location /moodledata/

            { internal; alias /path/to/moodledata/; }

            location /directfile/

            { internal; alias /path/to/http/root/directory/;}


            — snip —

            In the config file you will have to setup those locations:
            $CFG->dataroot_xaccelredirect = '/moodledata/';
            $CFG->dirroot_xaccelredirect = '/directfile/';

            The implementation should be compatible with "non nginx setups", except for one buffer flushing I had to remove in prepare_file_content_sending() [lib/filelib.php]. I'm not sure about the implications of this modification, so if you are familiar with the subject please comment.

            Thanks,

            Show
            Loic Jeannin added a comment - I've implemented a more "spread" use of nginx's X-Accel-Redirect against v2.2.2. You can find my initial work here http://http://git.digitalsk.com.br/moodle.git branch xaccel To make it work you will have to configure 2 internal locations in nginx, one for your root dir and one for moodleadta: — snip — location /moodledata/ { internal; alias /path/to/moodledata/; } location /directfile/ { internal; alias /path/to/http/root/directory/;} — snip — In the config file you will have to setup those locations: $CFG->dataroot_xaccelredirect = '/moodledata/'; $CFG->dirroot_xaccelredirect = '/directfile/'; The implementation should be compatible with "non nginx setups", except for one buffer flushing I had to remove in prepare_file_content_sending() [lib/filelib.php] . I'm not sure about the implications of this modification, so if you are familiar with the subject please comment. Thanks,
            Hide
            Hubert Chathi added a comment -

            Loic, I'm having problems fetching from your repository. When I try to do a git fetch, it just hangs.

            Anyways, I don't think that you need the /directfile/ rule. As far as I can tell, you should be able to just do "X-Accel-Redirect: /path/to/file/relative/to/http/root" if you're sending a file that is in the Moodle web directory.

            Show
            Hubert Chathi added a comment - Loic, I'm having problems fetching from your repository. When I try to do a git fetch, it just hangs. Anyways, I don't think that you need the /directfile/ rule. As far as I can tell, you should be able to just do "X-Accel-Redirect: /path/to/file/relative/to/http/root" if you're sending a file that is in the Moodle web directory.
            Hide
            Petr Skoda added a comment -

            Hi,

            finally I have installed xsendfile support for my test server, I am going to work on it this week, hopefully to be submitted for integration for next week (master). Thanks for all the patches and information!

            Petr

            Show
            Petr Skoda added a comment - Hi, finally I have installed xsendfile support for my test server, I am going to work on it this week, hopefully to be submitted for integration for next week (master). Thanks for all the patches and information! Petr
            Hide
            Petr Skoda added a comment - - edited

            notes:

            • we must not give away the filepool file path, instead I guess the easiest may be to move this inside the stored_file
            • I agree we can not use xsendfile when dontdie or temp file served - because we can not delete files afterserving
            • I like the idea of readfile_accel() that does the selected serving
            • prepare_file_content_sending() should be optionally called from readfile_accel()
            • it might be better to allow configuration from config.php only because we need to have the setting available also before we load the CFG from database
            • I did not know there are so many different xsendfile ways - I will definitely need testers
            Show
            Petr Skoda added a comment - - edited notes: we must not give away the filepool file path, instead I guess the easiest may be to move this inside the stored_file I agree we can not use xsendfile when dontdie or temp file served - because we can not delete files afterserving I like the idea of readfile_accel() that does the selected serving prepare_file_content_sending() should be optionally called from readfile_accel() it might be better to allow configuration from config.php only because we need to have the setting available also before we load the CFG from database I did not know there are so many different xsendfile ways - I will definitely need testers
            Hide
            Petr Skoda added a comment -

            Hmm, there are two major changes pending in master - image preview and repository linking, I am afraid this will have to wait a bit more, hopefully the feature freeze will not block this...

            Show
            Petr Skoda added a comment - Hmm, there are two major changes pending in master - image preview and repository linking, I am afraid this will have to wait a bit more, hopefully the feature freeze will not block this...
            Hide
            Loic Jeannin added a comment -

            Hubert,
            did you do : git clone http://git.digitalsk.com.br/moodle.git ? It should work.
            For the /directfile/ you might be right but I have not tested that way. I assumed I had to declare the location "internal;" to work with X-Accel-Redirect.

            Show
            Loic Jeannin added a comment - Hubert, did you do : git clone http://git.digitalsk.com.br/moodle.git ? It should work. For the /directfile/ you might be right but I have not tested that way. I assumed I had to declare the location "internal;" to work with X-Accel-Redirect.
            Hide
            Loic Jeannin added a comment -

            Hubert, I attached the patch (xaccel_v2.2.2.patch) in case you are still unable to clone my repository.

            Show
            Loic Jeannin added a comment - Hubert, I attached the patch (xaccel_v2.2.2.patch) in case you are still unable to clone my repository.
            Hide
            Hubert Chathi added a comment -

            Loic,
            I tried cloning again, and it still hangs. I'm not sure what's wrong. Thanks for attaching the patch, though.
            The "internal" is just a suggestion, to make sure that files that shouldn't be otherwise available aren't. So it isn't needed if nginx already serves that directory.
            Regarding your patch, a few comments:

            • it would be better to put your readfile_accel in an existing library file instead of creating a new one
            • you should follow the Moodle coding style (the thing I noticed was your indenting)
            • there's no guarantee that the Moodle data directory will contain the word "moodledata", or that something in the main Moodle directory won't contain "moodledata" in its filename (e.g. if someone decides for whatever reason to create a plugin with "moodledata" in the name). So it would be best to compare against the actual contents of $CFG->dataroot. The "else" should also explicitly check against dirroot, in case someone, for whatever reason, calls that function on a file that isn't in either directory.

            Comparing Loic's patch against Tomasz/my patch, I would say that Loic's patch is more thorough in that it also uses acceleration for theme files. Some benchmarks show that acceleration is less important for small files, so it's debatable how important it is for theme files. The downside to doing acceleration for theme files is that the theme PHP files use ABORT_AFTER_CONFIG, which means that it must be configured in config.php instead of in the database (or, at least, if it's configured in the database, the theme PHP files won't see the configuration). IMHO, I don't see a reason not to use acceleration in the theme files if it's possible, but we also want to allow users to configure those options within Moodle. So I would suggest that we add the options in the Moodle UI, but add a note saying that if they want theme files to be accelerated, then they need to set the options in the config.php file.

            BTW, I also now have access to an nginx server, so I can test both Lighttpd and nginx.

            Show
            Hubert Chathi added a comment - Loic, I tried cloning again, and it still hangs. I'm not sure what's wrong. Thanks for attaching the patch, though. The "internal" is just a suggestion, to make sure that files that shouldn't be otherwise available aren't. So it isn't needed if nginx already serves that directory. Regarding your patch, a few comments: it would be better to put your readfile_accel in an existing library file instead of creating a new one you should follow the Moodle coding style (the thing I noticed was your indenting) there's no guarantee that the Moodle data directory will contain the word "moodledata", or that something in the main Moodle directory won't contain "moodledata" in its filename (e.g. if someone decides for whatever reason to create a plugin with "moodledata" in the name). So it would be best to compare against the actual contents of $CFG->dataroot. The "else" should also explicitly check against dirroot, in case someone, for whatever reason, calls that function on a file that isn't in either directory. Comparing Loic's patch against Tomasz/my patch, I would say that Loic's patch is more thorough in that it also uses acceleration for theme files. Some benchmarks show that acceleration is less important for small files, so it's debatable how important it is for theme files. The downside to doing acceleration for theme files is that the theme PHP files use ABORT_AFTER_CONFIG, which means that it must be configured in config.php instead of in the database (or, at least, if it's configured in the database, the theme PHP files won't see the configuration). IMHO, I don't see a reason not to use acceleration in the theme files if it's possible, but we also want to allow users to configure those options within Moodle. So I would suggest that we add the options in the Moodle UI, but add a note saying that if they want theme files to be accelerated, then they need to set the options in the config.php file. BTW, I also now have access to an nginx server, so I can test both Lighttpd and nginx.
            Hide
            Loic Jeannin added a comment -

            > it would be better to put your readfile_accel in an existing library file instead of creating a new one

            which one do you suggest ?

            > you should follow the Moodle coding style (the thing I noticed was your indenting)

            Ok, my vim settings are ok now.

            > there's no guarantee that the Moodle data directory will contain the word "moodledata", or that
            >something in the main Moodle directory won't contain "moodledata" in its filename (e.g. if someone decides
            >for whatever reason to create a plugin with "moodledata" in the name). So it would be best to compare
            >against the actual contents of $CFG->dataroot. The "else" should also explicitly check against dirroot, in
            >case someone, for whatever reason, calls that function on a file that isn't in either directory.

            Right, I've already noticed that too.

            I'm going to rewrite following the coding style (tabs/spaced are already fixed) and post it here.

            Show
            Loic Jeannin added a comment - > it would be better to put your readfile_accel in an existing library file instead of creating a new one which one do you suggest ? > you should follow the Moodle coding style (the thing I noticed was your indenting) Ok, my vim settings are ok now. > there's no guarantee that the Moodle data directory will contain the word "moodledata", or that >something in the main Moodle directory won't contain "moodledata" in its filename (e.g. if someone decides >for whatever reason to create a plugin with "moodledata" in the name). So it would be best to compare >against the actual contents of $CFG->dataroot. The "else" should also explicitly check against dirroot, in >case someone, for whatever reason, calls that function on a file that isn't in either directory. Right, I've already noticed that too. I'm going to rewrite following the coding style (tabs/spaced are already fixed) and post it here.
            Hide
            Hubert Chathi added a comment -

            > it would be better to put your readfile_accel in an existing library file instead of creating a new one
            which one do you suggest ?

            Sorry, I thought that I erased that comment.
            Usually, I would suggest either moodlelib or weblib. But in this case, since you're calling the function from the theme PHP files, and it looks like there's a desire to keep those as light as possible, I don't know if that would work. So in this particular case, it might be fine the way you have it.

            Show
            Hubert Chathi added a comment - > it would be better to put your readfile_accel in an existing library file instead of creating a new one which one do you suggest ? Sorry, I thought that I erased that comment. Usually, I would suggest either moodlelib or weblib. But in this case, since you're calling the function from the theme PHP files, and it looks like there's a desire to keep those as light as possible, I don't know if that would work. So in this particular case, it might be fine the way you have it.
            Hide
            Loic Jeannin added a comment -

            version 0.2

            Show
            Loic Jeannin added a comment - version 0.2
            Hide
            Loic Jeannin added a comment -

            I've updated the git repository and the attached patch, now the coding style should be fixed.
            I have 2 issues remaining:

            • the buffer flushing is still commented (prepare_file_content_sending())
            • the stored_file->readfile() is still unaware of the "dontdie" option
            Show
            Loic Jeannin added a comment - I've updated the git repository and the attached patch, now the coding style should be fixed. I have 2 issues remaining: the buffer flushing is still commented (prepare_file_content_sending()) the stored_file->readfile() is still unaware of the "dontdie" option
            Hide
            Petr Skoda added a comment -

            Thanks everybody for the patch, while I was refactoring the file apis a bit I realised we could use the contenthash of stored files as etag and significantly improve performance of force downloaded files such as forum attachments.

            I have tested my patch with Apache X-Sendfile module only, hopefully it will work with Nginx and Lighttpd too.

            Please test and make some noise to help with getting this integrated.

            Petr

            Show
            Petr Skoda added a comment - Thanks everybody for the patch, while I was refactoring the file apis a bit I realised we could use the contenthash of stored files as etag and significantly improve performance of force downloaded files such as forum attachments. I have tested my patch with Apache X-Sendfile module only, hopefully it will work with Nginx and Lighttpd too. Please test and make some noise to help with getting this integrated. Petr
            Hide
            Martin Dougiamas added a comment -

            I assume that Petr's patch implements everything discussed above...

            Show
            Martin Dougiamas added a comment - I assume that Petr's patch implements everything discussed above...
            Hide
            Petr Skoda added a comment -

            yes, it should

            Show
            Petr Skoda added a comment - yes, it should
            Hide
            Loic Jeannin added a comment -

            Petr,
            The nginx part in readaccel is missing a way to add the prefix configured in nginx (internal or not) for a direct access to the dataroot.
            The way you wrote it should work OOTB with $dirroot but not with $dataroot.

            from your patch (line 61):
            $filepath = substr($filepath, strlen($dataroot));
            should be something like that:
            $filepath = $CFG->dataroot_nginxprefix . substr($filepath, strlen($dataroot));

            Thanks,

            Show
            Loic Jeannin added a comment - Petr, The nginx part in readaccel is missing a way to add the prefix configured in nginx (internal or not) for a direct access to the dataroot. The way you wrote it should work OOTB with $dirroot but not with $dataroot. from your patch (line 61): $filepath = substr($filepath, strlen($dataroot)); should be something like that: $filepath = $CFG->dataroot_nginxprefix . substr($filepath, strlen($dataroot)); Thanks,
            Hide
            Loic Jeannin added a comment -

            sorry, s/readaccel/xsendfilelib/

            Show
            Loic Jeannin added a comment - sorry, s/readaccel/xsendfilelib/
            Hide
            Petr Skoda added a comment -

            ah, the problem is that the cachedir and tempdir might be outside of $CFG->dataroot, I am afraid we will have to add more nginx prefixes

            Show
            Petr Skoda added a comment - ah, the problem is that the cachedir and tempdir might be outside of $CFG->dataroot, I am afraid we will have to add more nginx prefixes
            Hide
            Petr Skoda added a comment -

            I guess there is no way to simply whitelist absolute paths in nginx, right?

            Show
            Petr Skoda added a comment - I guess there is no way to simply whitelist absolute paths in nginx, right?
            Hide
            Petr Skoda added a comment - - edited

            I have added a new setting for the Nginx aliases (see config-dist.php), please test it.

            Show
            Petr Skoda added a comment - - edited I have added a new setting for the Nginx aliases (see config-dist.php), please test it.
            Hide
            Loic Jeannin added a comment -

            Sorry I don't understand what you mean with whitelist.
            X-Accel-Redirect is a bit more complicated than the other solutions. It does not work with file paths, only with url, so you have to configure all the locations you want to reach in your nginx config. I had'nt thought of the cases where cachedir/tempdir are outside the dataroot; in this case you will have to configure each dir separately and check in xsendfilelib in which dir the file is in to add the proper url prefix.
            I'm afraid there is no "easy way" to configure it.

            Thanks,

            Show
            Loic Jeannin added a comment - Sorry I don't understand what you mean with whitelist. X-Accel-Redirect is a bit more complicated than the other solutions. It does not work with file paths, only with url, so you have to configure all the locations you want to reach in your nginx config. I had'nt thought of the cases where cachedir/tempdir are outside the dataroot; in this case you will have to configure each dir separately and check in xsendfilelib in which dir the file is in to add the proper url prefix. I'm afraid there is no "easy way" to configure it. Thanks,
            Hide
            Loic Jeannin added a comment -

            Petr, looks great now.
            I'm testing

            Show
            Loic Jeannin added a comment - Petr, looks great now. I'm testing
            Hide
            Petr Skoda added a comment -

            Thanks for the explanation, I think I understood that correctly, my patch allows unlimited number of directory aliases which are selected automatically based on the real file path of the served path. Could somebody please verify it works? I do not have nginx install here. Or if somebody tells me how to configure nginx from macports on OSX I could do it myself...

            Show
            Petr Skoda added a comment - Thanks for the explanation, I think I understood that correctly, my patch allows unlimited number of directory aliases which are selected automatically based on the real file path of the served path. Could somebody please verify it works? I do not have nginx install here. Or if somebody tells me how to configure nginx from macports on OSX I could do it myself...
            Hide
            Loic Jeannin added a comment -

            Petr,
            It worked with this small adjustment:

            diff --git a/lib/xsendfilelib.php b/lib/xsendfilelib.php
            index 39106f7..6c78456 100644
            — a/lib/xsendfilelib.php
            +++ b/lib/xsendfilelib.php
            @@ -52,9 +52,11 @@ function xsendfile($filepath) {
            $aliased = false;
            if (!empty($CFG->xsendfilealiases) and is_array($CFG->xsendfilealiases)) {
            foreach ($CFG->xsendfilealiases as $alias=>$dir) {

            • $dir = realpath($dir).PATH_SEPARATOR;
              + $ds = DIRECTORY_SEPARATOR;
              + $dir = realpath($dir).DIRECTORY_SEPARATOR;
              if (strpos($filepath, $dir) === 0) { $filepath = $alias.substr($filepath, strlen($dir)); + $filepath = str_replace($ds.$ds, $ds, $filepath); $aliased = true; break; }


            The str_replace is just here to clean the path to be compared.

            Show
            Loic Jeannin added a comment - Petr, It worked with this small adjustment: diff --git a/lib/xsendfilelib.php b/lib/xsendfilelib.php index 39106f7..6c78456 100644 — a/lib/xsendfilelib.php +++ b/lib/xsendfilelib.php @@ -52,9 +52,11 @@ function xsendfile($filepath) { $aliased = false; if (!empty($CFG->xsendfilealiases) and is_array($CFG->xsendfilealiases)) { foreach ($CFG->xsendfilealiases as $alias=>$dir) { $dir = realpath($dir).PATH_SEPARATOR; + $ds = DIRECTORY_SEPARATOR; + $dir = realpath($dir).DIRECTORY_SEPARATOR; if (strpos($filepath, $dir) === 0) { $filepath = $alias.substr($filepath, strlen($dir)); + $filepath = str_replace($ds.$ds, $ds, $filepath); $aliased = true; break; } — The str_replace is just here to clean the path to be compared.
            Hide
            Loic Jeannin added a comment -

            The patch

            Show
            Loic Jeannin added a comment - The patch
            Hide
            Petr Skoda added a comment - - edited

            arrgh, separators again, thanks!!! Why are you replacing the separators in your patch? For me the realpath() removes all doubles and trims the trailing separator. It seems it might break only if you had '/' or "d:\" in alias, oh, but that is correct value too (but I guess not recommended) - fixed too.

            please retest, I have used a bit different logic that detects invalid dirs too.

            Show
            Petr Skoda added a comment - - edited arrgh, separators again, thanks!!! Why are you replacing the separators in your patch? For me the realpath() removes all doubles and trims the trailing separator. It seems it might break only if you had '/' or "d:\" in alias, oh, but that is correct value too (but I guess not recommended) - fixed too. please retest, I have used a bit different logic that detects invalid dirs too.
            Hide
            Loic Jeannin added a comment -

            Ok, test passed in linux env nginx + php_fpm. To be extensive we should find someone with a windows setup and nginx (good luck).
            On the actual code : it's much cleaner to test only the last char.

            Thanks,

            Show
            Loic Jeannin added a comment - Ok, test passed in linux env nginx + php_fpm. To be extensive we should find someone with a windows setup and nginx (good luck). On the actual code : it's much cleaner to test only the last char. Thanks,
            Hide
            Sam Hemelryk added a comment -

            Alrighty, this has been integrated now, thanks for the amazing effort everyone!

            Show
            Sam Hemelryk added a comment - Alrighty, this has been integrated now, thanks for the amazing effort everyone!
            Hide
            Ankit Agarwal added a comment -

            Results of Etag test:-
            -> Uploaded an image to forum.304 status is returned as expected on multiple page loads.
            -> Forum attachment was set to force download.

            Results of X-sendifle test:-

            X-sendfile set to on
            -> I got following strict standard errors all over the place( probably not related to above fix)

            Strict Standards: Creating default object from empty value in /var/www/int/master/moodle/grade/lib.php on line 225
            Strict Standards: Redefining already defined constructor for class grade_export_txt in /var/www/int/master/moodle/grade/export/txt/grade_export_txt.php on line 31 
            Strict Standards: Creating default object from empty value in /var/www/int/master/moodle/grade/lib.php on line 219
            Strict Standards: Creating default object from empty value in /var/www/int/master/moodle/mod/scorm/locallib.php on line 538 
            

            Passing
            Thanks

            Show
            Ankit Agarwal added a comment - Results of Etag test:- -> Uploaded an image to forum.304 status is returned as expected on multiple page loads. -> Forum attachment was set to force download. Results of X-sendifle test:- X-sendfile set to on -> I got following strict standard errors all over the place( probably not related to above fix) Strict Standards: Creating default object from empty value in /var/www/int/master/moodle/grade/lib.php on line 225 Strict Standards: Redefining already defined constructor for class grade_export_txt in /var/www/int/master/moodle/grade/export/txt/grade_export_txt.php on line 31 Strict Standards: Creating default object from empty value in /var/www/int/master/moodle/grade/lib.php on line 219 Strict Standards: Creating default object from empty value in /var/www/int/master/moodle/mod/scorm/locallib.php on line 538 Passing Thanks
            Hide
            Eloy Lafuente (stronk7) added a comment -

            UPDATE tracker_issues
               SET status = 'Closed',
                  comment = 'Thanks!'
            WHEN participants = 'Did a gorgeous work'
            

            This landed upstream some hours ago (some - me - developer fell slept in the sofa yesterday before spamming this).

            Show
            Eloy Lafuente (stronk7) added a comment - UPDATE tracker_issues SET status = 'Closed', comment = 'Thanks!' WHEN participants = 'Did a gorgeous work' This landed upstream some hours ago (some - me - developer fell slept in the sofa yesterday before spamming this).

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: