From bd9062bb8b409de5e7759bf588c1dbd0068f6067 Mon Sep 17 00:00:00 2001 From: Gervase Markham Date: Fri, 24 Oct 2014 09:17:24 +0100 Subject: Revert "Bug 1073264 - allow attachment download to be offloaded to the webserver using X-SendFile or equivalent. r=gerv, a=glob." Morning brain thought this bug was approved for 5.0. This reverts commit 55e8faeed19ff618483cb5803847bdba6c80c752. --- Bugzilla/Attachment.pm | 44 +--------------------- Bugzilla/Config/Attachment.pm | 8 ---- attachment.cgi | 18 +-------- .../en/default/admin/params/attachment.html.tmpl | 28 +------------- 4 files changed, 4 insertions(+), 94 deletions(-) diff --git a/Bugzilla/Attachment.pm b/Bugzilla/Attachment.pm index e165b139e..b44c94cd0 100644 --- a/Bugzilla/Attachment.pm +++ b/Bugzilla/Attachment.pm @@ -308,8 +308,7 @@ sub is_viewable { =item C -Returns the content of the attachment. -As a side-effect, sets $self->is_on_filesystem. +the content of the attachment =back @@ -326,16 +325,10 @@ sub data { undef, $self->id); - # Setting the property here is cheap, as opposed to making an extra - # query later, and hitting the filesystem to see if the file is - # still there. - $self->{is_on_filesystem} = 0; # If there's no attachment data in the database, the attachment is stored # in a local file, so retrieve it from there. if (length($self->{data}) == 0) { if (open(AH, $self->_get_local_filename())) { - # file is actually on disk. - $self->{is_on_filesystem} = 1; local $/; binmode AH; $self->{data} = ; @@ -348,36 +341,9 @@ sub data { =over -=item C - -Returns true if the attachment is stored on disk (via maxlocalattachment -parameter), as opposed to in the database. - -=back - -=cut - -# When the attachment is on the filesystem, you can let the backend -# (nginx, apache, lighttpd) serve it for you if it supports the X-Sendfile -# feature. This means that the attachment CGI script may have a reduced -# footprint. e.g. bug 906010 and bug 1073241. - -sub is_on_filesystem { - my $self = shift; - return $self->{is_on_filesystem} if exists $self->{is_on_filesystem}; - # In order to serve an attachment, you also send the datasize in the - # content-length header. Making additional queries which are exactly - # the same as found in the datasize code path is just wasteful. - my $datasize = $self->datasize; - return $self->{is_on_filesystem}; -} - -=over - =item C -Returns the length (in bytes) of the attachment content. -As a side-effect, sets $self->is_on_filesystem. +the length (in bytes) of the attachment content =back @@ -404,17 +370,11 @@ sub datasize { WHERE id = ?", undef, $self->id) || 0; - # Setting the property here is cheap, as opposed to making an extra - # query later, and hitting the filesystem to see if the file is - # still there. - $self->{is_on_filesystem} = 0; # If there's no attachment data in the database, either the attachment # is stored in a local file, and so retrieve its size from the file, # or the attachment has been deleted. unless ($self->{datasize}) { if (open(AH, $self->_get_local_filename())) { - # file is actually on disk. - $self->{is_on_filesystem} = 1; binmode AH; $self->{datasize} = (stat(AH))[7]; close(AH); diff --git a/Bugzilla/Config/Attachment.pm b/Bugzilla/Config/Attachment.pm index 5bf854293..580ec46d9 100644 --- a/Bugzilla/Config/Attachment.pm +++ b/Bugzilla/Config/Attachment.pm @@ -37,14 +37,6 @@ sub get_param_list { default => 0 }, - { - name => 'xsendfile_header', - type => 's', - choices => ['off', 'X-Sendfile', 'X-Accel-Redirect', 'X-LIGHTTPD-send-file'], - default => 'off', - checker => \&check_multi - }, - { name => 'maxattachmentsize', type => 't', diff --git a/attachment.cgi b/attachment.cgi index 61e6f58d8..5db8f5909 100755 --- a/attachment.cgi +++ b/attachment.cgi @@ -26,7 +26,6 @@ use Bugzilla::Attachment::PatchReader; use Bugzilla::Token; use Encode qw(encode find_encoding); -use Cwd qw(realpath); # For most scripts we don't make $cgi and $template global variables. But # when preparing Bugzilla for mod_perl, this script used these @@ -362,24 +361,9 @@ sub view { } } } - my $sendfile_header = {}; - my $sendfile_param = Bugzilla->params->{'xsendfile_header'}; - if ($attachment->is_on_filesystem && $sendfile_param ne 'off') { - # attachment is on filesystem and Admin turned on feature. - # This means we can let webserver handle the request and stream the file - # for us. This is called the X-Sendfile feature. see bug 1073264. - my $attachment_path = realpath($attachment->_get_local_filename()); - $sendfile_header->{$sendfile_param} = $attachment_path; - } print $cgi->header(-type=>"$contenttype; name=\"$filename\"", -content_disposition=> "$disposition; filename=\"$filename\"", - -content_length => $attachment->datasize, - %$sendfile_header); - if ($attachment->is_on_filesystem && $sendfile_param ne 'off') { - # in case of X-Sendfile, we do not print the data. - # that is handled directly by the webserver. - return; - } + -content_length => $attachment->datasize); disable_utf8(); print $attachment->data; } diff --git a/template/en/default/admin/params/attachment.html.tmpl b/template/en/default/admin/params/attachment.html.tmpl index c850802ab..5efcc1106 100644 --- a/template/en/default/admin/params/attachment.html.tmpl +++ b/template/en/default/admin/params/attachment.html.tmpl @@ -58,31 +58,5 @@ maxlocalattachment => "The maximum size (in megabytes) of attachments to be stored " _ "locally on the web server. If set to a value lower than the " _ "maxattachmentsize parameter, " _ - "attachments will never be kept on the local filesystem. " _ - "If you want to store all attachments on disk rather than in the " _ - "database, then set " _ - "maxattachmentsize parameter to 0. ", - - - xsendfile_header => - "By default, attachments are served by the CGI script. " _ - "If you enable filesystem file storage for large files using the " _ - "maxlocalattachment parameter " _ - "then you can have those files served directly by the webserver, which " _ - "avoids copying them entirely into memory, and this may result in a " _ - "performance improvement. To do this, configure your webserver appropriately " _ - "and then set the correct header, as follows:" _ - "
" _ - "Please note that attachments stored in the database cannot be offloaded " _ - "to apache/nginx/lighttpd and are always handled by the CGI script." - - } - + "attachments will never be kept on the local filesystem." } %] -- cgit v1.2.1