summaryrefslogtreecommitdiffstats
path: root/Bugzilla/Attachment
diff options
context:
space:
mode:
authorByron Jones <bjones@mozilla.com>2014-01-14 05:36:27 +0100
committerByron Jones <bjones@mozilla.com>2014-01-14 05:36:27 +0100
commit7ff7c6c8add474d9ea46ef7cc6928968d56ce449 (patch)
tree629c5f10d9aa525602b9d6564e91dcbd79e78782 /Bugzilla/Attachment
parent9a5f59187a2be3f744efc5e4e33e6bd956d9aa89 (diff)
downloadbugzilla-7ff7c6c8add474d9ea46ef7cc6928968d56ce449.tar.gz
bugzilla-7ff7c6c8add474d9ea46ef7cc6928968d56ce449.tar.xz
Bug 845725: interdiff hangs on massive patches
r=dkl, a=sgreen
Diffstat (limited to 'Bugzilla/Attachment')
-rw-r--r--Bugzilla/Attachment/PatchReader.pm90
1 files changed, 63 insertions, 27 deletions
diff --git a/Bugzilla/Attachment/PatchReader.pm b/Bugzilla/Attachment/PatchReader.pm
index b29240c5b..c2ef53a65 100644
--- a/Bugzilla/Attachment/PatchReader.pm
+++ b/Bugzilla/Attachment/PatchReader.pm
@@ -10,6 +10,8 @@ package Bugzilla::Attachment::PatchReader;
use 5.10.1;
use strict;
+use Config;
+use IO::Select;
use IPC::Open3;
use Symbol 'gensym';
@@ -17,6 +19,8 @@ use Bugzilla::Error;
use Bugzilla::Attachment;
use Bugzilla::Util;
+use constant PERLIO_IS_ENABLED => $Config{useperlio};
+
sub process_diff {
my ($attachment, $format, $context) = @_;
my $dbh = Bugzilla->dbh;
@@ -30,8 +34,7 @@ sub process_diff {
require PatchReader::DiffPrinter::raw;
$last_reader->sends_data_to(new PatchReader::DiffPrinter::raw());
# Actually print out the patch.
- print $cgi->header(-type => 'text/plain',
- -expires => '+3M');
+ print $cgi->header(-type => 'text/plain');
disable_utf8();
$reader->iterate_string('Attachment ' . $attachment->id, $attachment->data);
}
@@ -102,9 +105,12 @@ sub process_interdiff {
# Send through interdiff, send output directly to template.
# Must hack path so that interdiff will work.
- $ENV{'PATH'} = $lc->{diffpath};
+ local $ENV{'PATH'} = $lc->{diffpath};
- my ($pid, $interdiff_stdout, $interdiff_stderr);
+ # Open the interdiff pipe, reading from both STDOUT and STDERR
+ # To avoid deadlocks, we have to read the entire output from all handles
+ my ($stdout, $stderr) = ('', '');
+ my ($pid, $interdiff_stdout, $interdiff_stderr, $use_select);
if ($ENV{MOD_PERL}) {
require Apache2::RequestUtil;
require Apache2::SubProcess;
@@ -112,37 +118,73 @@ sub process_interdiff {
(undef, $interdiff_stdout, $interdiff_stderr) = $request->spawn_proc_prog(
$lc->{interdiffbin}, [$old_filename, $new_filename]
);
+ $use_select = !PERLIO_IS_ENABLED;
} else {
$interdiff_stderr = gensym;
- my $pid = open3(gensym, $interdiff_stdout, $interdiff_stderr,
+ $pid = open3(gensym, $interdiff_stdout, $interdiff_stderr,
$lc->{interdiffbin}, $old_filename, $new_filename);
+ $use_select = 1;
}
- binmode $interdiff_stdout;
- # Check for errors
- {
- local $/ = undef;
- my $error = <$interdiff_stderr>;
- if ($error) {
- warn($error);
- $warning = 'interdiff3';
+ if ($format ne 'raw' && Bugzilla->params->{'utf8'}) {
+ binmode $interdiff_stdout, ':utf8';
+ binmode $interdiff_stderr, ':utf8';
+ } else {
+ binmode $interdiff_stdout;
+ binmode $interdiff_stderr;
+ }
+
+ if ($use_select) {
+ my $select = IO::Select->new();
+ $select->add($interdiff_stdout, $interdiff_stderr);
+ while (my @handles = $select->can_read) {
+ foreach my $handle (@handles) {
+ my $line = <$handle>;
+ if (!defined $line) {
+ $select->remove($handle);
+ next;
+ }
+ if ($handle == $interdiff_stdout) {
+ $stdout .= $line;
+ } else {
+ $stderr .= $line;
+ }
+ }
}
+ waitpid($pid, 0) if $pid;
+
+ } else {
+ local $/ = undef;
+ $stdout = <$interdiff_stdout>;
+ $stdout //= '';
+ $stderr = <$interdiff_stderr>;
+ $stderr //= '';
}
- my ($reader, $last_reader) = setup_patch_readers("", $context);
+ close($interdiff_stdout),
+ close($interdiff_stderr);
+
+ # Tidy up
+ unlink($old_filename) or warn "Could not unlink $old_filename: $!";
+ unlink($new_filename) or warn "Could not unlink $new_filename: $!";
+
+ # Any output on STDERR means interdiff failed to full process the patches.
+ # Interdiff's error messages are generic and not useful to end users, so we
+ # show a generic failure message.
+ if ($stderr) {
+ warn($stderr);
+ $warning = 'interdiff3';
+ }
+ my ($reader, $last_reader) = setup_patch_readers("", $context);
if ($format eq 'raw') {
require PatchReader::DiffPrinter::raw;
$last_reader->sends_data_to(new PatchReader::DiffPrinter::raw());
# Actually print out the patch.
- print $cgi->header(-type => 'text/plain',
- -expires => '+3M');
+ print $cgi->header(-type => 'text/plain');
disable_utf8();
}
else {
- # In case the HTML page is displayed with the UTF-8 encoding.
- binmode $interdiff_stdout, ':utf8' if Bugzilla->params->{'utf8'};
-
$vars->{'warning'} = $warning if $warning;
$vars->{'bugid'} = $new_attachment->bug_id;
$vars->{'oldid'} = $old_attachment->id;
@@ -152,14 +194,8 @@ sub process_interdiff {
setup_template_patch_reader($last_reader, $format, $context, $vars);
}
- $reader->iterate_fh($interdiff_stdout, 'interdiff #' . $old_attachment->id .
- ' #' . $new_attachment->id);
- waitpid($pid, 0) if $pid;
- $ENV{'PATH'} = '';
-
- # Delete temporary files.
- unlink($old_filename) or warn "Could not unlink $old_filename: $!";
- unlink($new_filename) or warn "Could not unlink $new_filename: $!";
+ $reader->iterate_string('interdiff #' . $old_attachment->id .
+ ' #' . $new_attachment->id, $stdout);
}
######################