Edgewall Software
Modify

Opened 6 years ago

Closed 2 years ago

#7894 closed enhancement (fixed)

Add ability to export raw-attachments directly via apache

Reported by: anonymous Owned by: rblank
Priority: low Milestone: 1.0
Component: attachment Version: none
Severity: normal Keywords: attachment raw-attachment
Cc: mmitar@…
Release Notes:

Added support for X-Sendfile for files served from the filesystem.

API Changes:

Description (last modified by rblank)

In my environment I have many projects with many users, many of whom use the wiki attachments to upload/download the software release tarballs. It's not practical to have them ssh into the server to upload files to another area (which could be exported directly by apache), and using wiki attachments is just very convenient and neat. However with lots of downloads it would be nice to decrease the load on the server.

Since apache is able to serve static files more efficiently than trac, it would be nice if the raw-attachments directory could be aliased in the apache config. This can of course be done with the main trac static css/image files via "trac-admin deploy" and apache Alias. However unfortunately the files in the attachments directory have filenames which are encoded, so an alias like the following won't work for all files (like ones with spaces or the like):

AliasMatch ^/trac/([a-z0-9_]+)/raw-attachment/(.*) /data/trac/$1/attachments/$2

Since the only way I can see to do this is to have the files on the disk saved without encoding, it would seem to be a bit difficult to implement since we wouldn't want to break files which were already uploaded. Possibly just new file uploads (or files in newly created trac projects) could be unencoded so that nothing broke for users who upgraded their previous trac environments. Then for those who have older trac environments and want to use the apache alias, a script could be used to rename the files and remove the uuencoding.

Attachments (3)

7894-x-sendfile-r10774.patch (3.2 KB) - added by rblank 3 years ago.
Add support for X-Sendfile in req.send_file().
7894-x-sendfile-2-r10774.patch (3.3 KB) - added by rblank 3 years ago.
Send header before end_headers().
7894-x-sendfile-r11053.patch (3.4 KB) - added by rblank 2 years ago.
Add support for X-Sendfile in req.send_file().

Download all attachments as: .zip

Change History (19)

comment:1 Changed 6 years ago by Graham.Dumpleton@…

How are you hosting Trac? If using mod_wsgi then Trac should be using the wsgi.file_wrapper extension to serve up the attachments more efficiently than if Trac did it itself. What the wsgi.file_wrapper does is allow Apache/mod_wsgi to use sendfile() or memory mapping techniques in sending the file. The performance isn't as good as Apache serving static files, but not that far short that would generally be a problem for stuff like attachment downloading, which overall isn't going to be as frequent as static media files such as style sheets etc.

comment:2 Changed 6 years ago by rblank

  • Description modified (diff)
  • Keywords needinfo added
  • Milestone 0.12 deleted

A direct performance comparison could be done between direct serving using the AliasMatch mentioned above, and Graham's suggestion in comment:1, by testing with file names that don't require encoding.

As fixing this would require a backwards-incompatible change, I'd like to be sure that the performance gain is significant.

comment:3 follow-up: Changed 6 years ago by cboos

  • Keywords needinfo removed
  • Milestone set to 2.0

Trac could eventually send redirects in this case, with the target URL being e.g. /static-attachment/<realm>/<encoded-id>/<encoded-file>, which would allow to write an AliasMatch pointing to the encoded files.

comment:4 Changed 5 years ago by cboos

  • Milestone changed from 2.0 to unscheduled

Milestone 2.0 deleted

comment:5 in reply to: ↑ 3 Changed 4 years ago by rblank

  • Milestone changed from triaging to unscheduled

Replying to cboos:

Trac could eventually send redirects in this case, with the target URL being e.g. /static-attachment/<realm>/<encoded-id>/<encoded-file>, which would allow to write an AliasMatch pointing to the encoded files.

Nice idea, and probably quite easy to implement. I'd still like to see a performance comparison as suggested in comment:2 before working on this. Also, this would bypass the permission checks, so everybody who has access to attachments has access to all attachments.

comment:6 Changed 3 years ago by Mitar

  • Cc mmitar@… added

Why not use X-Sendfile header (or respective other things for other HTTP servers)? So Trac would send an empty response (if user has permissions) with X-Sendfile header line pointing to the file Apache (or some other HTTP server) has to send.

This could be make extendable, so that support for other HTTP servers can be added through plugins.

Similar is done for example in django-filer: default sending through Django vs. X-Sendfile. User then selects which option she wants. Documentation.

comment:7 Changed 3 years ago by rblank

  • Milestone changed from unscheduled to next-major-0.1X

X-Sendfile looks like a very good idea indeed. We could use that in req.send_file() (activated by a config option). Would you like to try and implement it?

comment:8 Changed 3 years ago by Mitar

I can maybe add this to my lowpriority TODO. ;-)

You can assign me the ticket, if you want.

comment:9 Changed 3 years ago by rblank

  • Milestone changed from next-major-0.1X to 0.13
  • Owner set to rblank

Ok, let's try to get this into 0.13.

Changed 3 years ago by rblank

Add support for X-Sendfile in req.send_file().

comment:10 follow-up: Changed 3 years ago by rblank

7894-x-sendfile-r10774.patch adds support for X-Sendfile. It must be enabled explicitly by setting [trac] use_xsendfile = true in trac.ini.

Unfortunately, I don't have a setup at hand to test the functionality. Mitar, do you happen to have a web server supporting X-Sendfile? Could you please test the patch?

nginx has a similar functionality but with a different header X-Accel-Redirect. I'm not sure we can easily support it as well, without making the configuration more complex. Actually, I'm not even sure I understand how it works. The page above says the header should contain a URI, but then goes on to show a path, one that is absolute but is interpreted as relative. Weird.

comment:11 in reply to: ↑ 10 ; follow-up: Changed 3 years ago by cboos

In the patch, the send_header(...) is done too late, it should be done before the call to end_headers().

Replying to rblank:

nginx has […] X-Accel-Redirect. I'm not sure we can easily support it as well, without making the configuration more complex.

We could have a different option, use_xaccelredirect, accepting a relative path.

Actually, I'm not even sure I understand how it works. The page above says the header should contain a URI, but then goes on to show a path, one that is absolute but is interpreted as relative. Weird.

As I understand it, if we have e.g. use_xaccelredirect == '/tracenv1/attachments' and e.g. the following setup in nginx:

location /tracenv1/attachments/ {
  internal;
  root   /var/tracenvs;
}

then we would have to send headers like X-Accel-Redirect: /tracenv1/attachments/wiki/WikiStart/test.png; and it will send the /var/tracenvs/tracenv1/attachements/wiki/WikiStart/test.png file.

But of course, this could be done later (for example, with a patch contributed by an nginx user!)

Changed 3 years ago by rblank

Send header before end_headers().

comment:12 in reply to: ↑ 11 ; follow-up: Changed 3 years ago by rblank

Replying to cboos:

In the patch, the send_header(...) is done too late, it should be done before the call to end_headers().

Oh, right. Fixed in 7894-x-sendfile-2-r10774.patch.

As I understand it, if we have e.g. use_xaccelredirect == '/tracenv1/attachments' and e.g. the following setup in nginx:

Well, that's where it gets hairy. We also use send_file() to send static resources, which won't necessarily be below the root. That's not too bad, as it's better to have the resources sent by the web server directly anyway. But plugins may also use send_file(), and in that case, we can't know where they serve files from. We could probably set the [trac] use_xaccelredirect option to the root configured, and only use X-Accel-Redirect if the file to be sent is below the root, or mandate root = /, but that's probably not acceptable.

Anyway, I'd rather have this contributed by someone familiar with nginx. And I still need a tester for X-Sendfile :)

comment:13 Changed 3 years ago by Mitar

Why wouldn't this be extendable? Provide some API and an extension point? So that new HTTP servers could be provided by plugins (and some of them can be provided officially)? For example, some really heavy user could have a bunch of servers and would maybe want to load-balance between them, so this user could write a plugin which would point the user to the right content server.

So maybe just something like:

send_file_method = NativeMethod

this is copying the file in Python.

And for Apache:

send_file_method = XSendfileMethod

Where both NativeMethod? and XSendfileMethod are Trac components with proper interface.

comment:14 in reply to: ↑ 12 Changed 3 years ago by Mitar

Replying to rblank:

We could probably set the [trac] use_xaccelredirect option to the root configured, and only use X-Accel-Redirect if the file to be sent is below the root, or mandate root = /, but that's probably not acceptable.

The same applies also to the xsendfile. Apache could be configured to allow xsendfile only for some directories (in general, a very smart idea anyway). So we cannot just assume we can send all files like that. Especially for plugin files which are extracted from some egg somewhere user might now know here and has not specified this location in Apache.

But I think this is a great think to solve the problem that currently plugin files were always served only by Trac and you couldn't do the alias thing as for the Trac's and user's htdocs.

Changed 2 years ago by rblank

Add support for X-Sendfile in req.send_file().

comment:15 Changed 2 years ago by rblank

I have refreshed the patch for current trunk, but I unfortunately still don't have a setup to actually test the functionality. If someone could test the patch and confirm that it works properly, this could land in 1.0.

comment:16 Changed 2 years ago by rblank

  • Release Notes modified (diff)
  • Resolution set to fixed
  • Status changed from new to closed

I finally managed to set up a test Apache with mod_xsendfile, and the functionality works as advertised. Patch applied in [11071].

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain rblank.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from rblank to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.