Support for X-Sendfile

[evert]

Original author: ncas...@gmail.com (August 13, 2010 08:17:08)

SabreDAV should support the X-Sendfile header. This would improve file delivery performance. See:

• http://blog.lighttpd.net/articles/2006/07/02/x-sendfile (lighttpd)
• http://tn123.ath.cx/mod_xsendfile/ (Apache)

Original issue: http://code.google.com/p/sabredav/issues/detail?id=69

[evert]

From evert...@gmail.com on August 13, 2010 12:58:04: I think this is a good idea. This would most definitely be a plugin.

[evert]

From toupeir...@gmail.com on September 30, 2010 10:28:05: Hi there, I wanted to ask if you already started work on this. If not I'll give it a try, I think it could a nice performance improvement for our site.

[evert]

From evert...@gmail.com on September 30, 2010 11:09:53: I have not, but I welcome the addition.

The way I would implement this, is by adding a new interface to File classes, such as :

interface Sabre_DAV_XSendFile_IFile extends Sabre_DAV_IFile {

function getPhysicalPath();

}

And then create a Sabre_DAV_XSendFile_Plugin that intercepts GET requests, checks if the node implements that interface, and set the appropriate header.

[evert]

From toupeir...@gmail.com on October 04, 2010 19:05:28: Okay thanks, I'll have a look at this in the coming weeks and will gladly follow your guidelines!

[evert]

From toupeir...@gmail.com on July 06, 2011 10:54:47: I just had a go at this, see the attached patch. It works fine with our application, using Apache 2.2 and mod_xsendfile 0.9.

I'd appreciate if you could have a look at the code, and possibly include it in the next release.

[evert]

From evert...@gmail.com on July 07, 2011 03:29:36: Looks like a great patch. I can add this in, but it will be the release after the next

[evert]

From toupeir...@gmail.com on July 07, 2011 13:52:03: No problem, thanks for adding it!

Also, I'm not sure about that TODO which I just copied from Sabre_DAV_Server#httpGet().

[evert]

From evert...@gmail.com on August 03, 2011 05:07:51: Won't be in the next release, but soon hopefully anyway. The next release is a bit too large already, and don't want to add too much new features all at once.

[evert]

From mackatack on May 04, 2012 08:40:10: Could you also implement nginx' feature called X-Accel-Redirect? see http://wiki.nginx.org/XSendfile Should be nothing more than just coping the X-Sendfile header and naming it X-Accel-Redirect. Thanks

[evert]

From toupeir...@gmail.com on May 04, 2012 15:42:45: I've switched jobs in the meantime and unfortunately haven't worked with SabreDAV in quite a while, so I hope Evert will add X-Accel-Redirect support :)

[evert]

From evert...@gmail.com on May 04, 2012 16:30:21: I like it, but would require a different or additional interface. The big difference is that one is a local path, and the other a local url.

[evert]

From mackatack on May 04, 2012 20:46:54: One could just configure nginx to accept the local path for internal redirects. Havnt tested it, but should work in theory.

[jcfischer]

Is this something that will be worked on or is this essentially dead? We are using ownCloud and see long request times, when there is a DAV transfer of a large file from the server to a client. @evert

[staabm]

@jcfischer do you have experience how php can detect if the server supports this header and also benchmarked in a real world app if it has a actual benefit?

[staabm]

for completlyness:

NGiNX also supports it but has a different name for the header need to be sent X-Accel-Redirect http://wiki.nginx.org/XSendfile

Symfony also supports this header, see http://symfony.com/doc/current/components/http_foundation/introduction.html#serving-files

[jcfischer]

@staabm sure - check out the OwnCloud source:

Adding the header: https://github.com/owncloud/core/blob/master/lib/private/files.php#L167

Here's a screenshot that shows that SabreDAV is spending over 16000 ms to send a (large) file.

By using the x-sendfile header, the webserver would be in charge of that and it the file (800MB in that case) wouldn't have to be loaded into memory...

[staabm]

sounds like a cool addition to me.

[evert]

I'd still like this. I have a few other changes that could benefit from this

[jcfischer]

what can we do to help drive this forward? (Except for coding - unfortunately (or fortunately?) I'm not a PHP developer

[evert]

I just created a new branch for this:

https://github.com/fruux/sabre-dav/tree/sendfile

Want to help? Help test!

A simple fileserver:

1. mkdir public
2. chmod 777 public
3. create server.php, containing the following:

<?php

namespace Sabre\DAV;

date_default_timezone_set('UTC');

$root = new FSExt\Collection(__DIR__ . '/public');
$server = new Server($root);
$server->addPlugin(
   new XSendFile\Plugin()
);
$server->exec();

?>

1. Make sure you access this as something like http://localhost/~mydir/sabredav/server.php/ and open it in a webdav client.
2. Let me know if it worked ;)

[evert]

Note that @jcfischer's assumption that anything needs to be buffered in memory is wrong though, unless owncloud doesn't correctly use streams (which I believe it does).

[staabm]

@evert is right that php doesn't need to load the whole file into memory. Nevertheless a webserver would handle the filetransfer in a more efficient manner

[staabm]

sendfile will be a big win mostly when php is not part of the webserver (mod_php) but connected with CGI/FCGI (which is the recommended php setup, afair)

[PVince81]

Nice plugin.

You might want to also add "X-Sendfile2" and "X-Accel-Redirect" too, if that makes sense, like https://github.com/owncloud/core/blob/master/lib/private/files.php#L177

[PVince81]

The current implementation seems to assume that whoever is using the plugin must decide whether to enable it or not. Another approach would be to make the plugin auto-detect the situation based on either some given headers like "MOD_X_SENDFILE_ENABLED" or let the caller set flags through its API or constructor.

[dratini0]

Would you mind if I tried to implement something lile what is in owncloud? Maybe with a bit more flexibility.

[evert]

Not at all, but it has to fit the architectural design. If you have an idea about an approach, share! :) i want to release 2.2 soon so it would be nice to include it

[dratini0]

The basic idea would be to have two optional parameters on the plugin's constructor, $headerName and $filenamePrefix. If these are present, the httpGet will just use the given header and prepend the prefix to the filename. If they are not given, httpGet will try to guess the right course of action by using the same environmental variables owncloud uses: MOD_X_SENDFILE_ENABLED, MOD_X_SENDFILE2_ENABLED, MOD_X_ACCEL_REDIRECT_ENABLED, MOD_X_ACCEL_REDIRECT_PREFIX. I think httpGet is the first time the request is available, but I will have to verify that. Of course I will be using Request::getRawServerValue.

[dratini0]

Hmm, I will have to rethink this, since servers are so largely dirrerent. Apache is a walk in the park, but lighthttpd needs workarounds for range support, while nginx is probably better off with a prefixing than with anything else. Therefore what I propose would be having httpGet fire an event with the filename and the request to some autodetectors which call their own helpers with custom data. If the server user wants to disable autodetection, they can pass the server's name and an array to the constructor, and the httpGet will call the appropriate helper function, probably using events again. Since I am bad at explaining, I will implement it for apache pretty soon.

[divinity76]

Note that @jcfischer's assumption that anything needs to be buffered in memory is wrong though, unless owncloud doesn't correctly use streams (which I believe it does).

actually it's PHP's fault, php's implementation of stream_copy_to_stream() works on a read()/write() loop and is kindof shitty implemented to boot (ref1) (ref2), it got some improvements in PHP 7.4 (not yet released, NEWS file calls it . Fixed bug #77930 (stream_copy_to_stream should use mmap more often)), but it's still not using sendfile() (which is the optimal way to transfer content between 2 handles without caring about the contents, at least in the Linux and *BSD world, Nginx does this correctly)

• the optimal userland implementation in PHP should have been using readfile() or stream_copy_to_stream(), but because of shitty php interpreter implementations of these functions, at least until php 7.4, that's not always the case (see https://github.com/sabre-io/http/pull/119 ) )

• in any case, using X-Sendfile / X-accel-redirect gives better performance than anything you could hope to achieve in PHP anyway, even if PHP was using sendfile() (which it isn't) :)

[staabm]

thx for your input.

I guess you should open a php-src bug (in case none is existant yet, for sendfile support).

regarding sabre-dav, I guess all is said in one of the above comments in this thread:

I just created a new branch for this:

fruux/sabre-dav@sendfile

Want to help? Help test!

A simple fileserver:

1. `mkdir public`

2. `chmod 777 public`

3. create `server.php`, containing the following:

<?php

namespace Sabre\DAV;

date_default_timezone_set('UTC');

$root = new FSExt\Collection(__DIR__ . '/public');
$server = new Server($root);
$server->addPlugin(
   new XSendFile\Plugin()
);
$server->exec();

?>

1. Make sure you access this as something like `http://localhost/~mydir/sabredav/server.php/` and open it in a webdav client.

2. Let me know if it worked ;)

[yevon]

Hi any news on this one? This would be really interesting. Not really clear if sabre can do anything about this, or maybe it's better to report it to the native php functions?