Skip to content

Commit

Permalink
Simplify error handling of downloader, avoid confusing error codes
Browse files Browse the repository at this point in the history 
* Remove the made-up HTTP response codes introduced by
  aea93ad (e.g. just log `Connect timeout`
  instead of `521 Connect timeout`)
* Simplify the code
* Keep retry behavior as-is, so if there's no response code at all or a
  real 5xx error code the downloader tries again
* Simplify code
* See https://progress.opensuse.org/issues/164222
  • Loading branch information
@Martchus
Martchus committed Jul 31, 2024
1 parent f7f8498 commit 85500a2
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 35 deletions.
42 changes: 18 additions & 24 deletions lib/OpenQA/Downloader.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,29 +23,26 @@ sub download ($self, $url, $target, $options = {}) {

local $ENV{MOJO_TMPDIR} = $self->tmpdir;

my $n = $self->attempts;
my ($err, $ret);
my $remaining_attempts = $self->attempts;
my ($code, $err);
while (1) {
$options->{on_attempt}->() if $options->{on_attempt};
($code, $err) = $self->_get($url, $target, $options);
return undef unless defined $err;

($ret, $err) = $self->_get($url, $target, $options);
return undef unless $ret;

if ($ret =~ /^5[0-9]{2}$/ && --$n) {
if ((--$remaining_attempts) && (!defined $code || ($code =~ /^5[0-9]{2}$/))) {
my $time = $self->sleep_time;
$log->info("Download error $ret, waiting $time seconds for next try ($n remaining)");
my $short_error_message = $code ? "Download error $code" : 'Download error';
$log->info("$short_error_message, waiting $time seconds for next try ($remaining_attempts remaining)");
sleep $time;
next;
}
elsif (!$n) {
elsif (!$remaining_attempts) {
$options->{on_failed}->() if $options->{on_failed};
last;
}

last;
}

return $err ? $err : 'No error message recorded';
return $err;
}

sub _extract_asset ($self, $to_extract, $target) {
Expand Down Expand Up @@ -85,19 +82,18 @@ sub _get ($self, $url, $target, $options) {
my $res = $tx->res;
$self->res($res);

my $code = $res->code // 521; # Used by cloudflare to indicate web server is down.
if ($code eq 304) {
my $code = $res->code;
my $error = $res->error // {message => 'Unknown error'};
if (defined $code && $code == 304) {
$options->{on_unchanged}->() if $options->{on_unchanged};
return (520, 'Unknown error') unless -e $target; # Avoid race condition between check and removal
return (undef, undef);
return (undef, -e $target ? undef : $error->{message});
}

if (!$res->is_success) {
my $error = $res->error;
my $message = ref $error eq 'HASH' ? " $error->{message}" : '';
my $log_err = qq{Download of "$target" failed: $code$message};
$log->info($log_err);
return ($code, $log_err);
my $error_message = defined $code ? "$code $error->{message}" : $error->{message};
my $log_message = qq{Download of "$target" failed: $error_message};
$log->info($log_message);
return ($code, $log_message);
}

unlink $target;
Expand All @@ -106,8 +102,7 @@ sub _get ($self, $url, $target, $options) {
my $asset = $res->content->asset;
my $size = $asset->size;
my $headers = $res->headers;
my $ret;
my $err;
my ($ret, $err);
if ($size == $headers->content_length) {
if ($options->{extract}) {
my $tempfile = path($ENV{MOJO_TMPDIR}, Mojo::URL->new($url)->path->parts->[-1])->to_string;
Expand All @@ -131,7 +126,6 @@ sub _get ($self, $url, $target, $options) {
my $actual_size = human_readable_size($size);
$err = qq{Size of "$target" differs, expected $header_size but downloaded $actual_size};
$log->info($err);
$ret = 598; # 598 (Informal convention) Network read timeout error
}

return ($ret, $err);
Expand Down
22 changes: 11 additions & 11 deletions t/25-downloader.t
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,15 +69,15 @@ $ua->connect_timeout(0.25)->inactivity_timeout(0.25);

subtest 'Connection refused' => sub {
my $from = "http://$host/tests/922756/asset/hdd/[email protected]";
like $downloader->download($from, $to), qr/Download of "$to" failed: 521/, 'Failed';
like $downloader->download($from, $to), qr/Download of "$to" failed: Connection refused/, 'Failed';

ok !-e $to, 'File not downloaded';

like $cache_log, qr/Downloading "test.qcow" from "$from"/, 'Download attempt';
like $cache_log, qr/Download of "$to" failed: 521/, 'Real error is logged';
like $cache_log, qr/Download error 521, waiting .* seconds for next try \(2 remaining\)/, '2 tries remaining';
like $cache_log, qr/Download error 521, waiting .* seconds for next try \(1 remaining\)/, '1 tries remaining';
unlike $cache_log, qr/Download error 521, waiting .* seconds for next try \(3 remaining\)/, 'only 3 attempts';
like $cache_log, qr/Download of "$to" failed: Connection refused/, 'Real error is logged';
like $cache_log, qr/Download error, waiting .* seconds for next try \(2 remaining\)/, '2 tries remaining';
like $cache_log, qr/Download error, waiting .* seconds for next try \(1 remaining\)/, '1 tries remaining';
unlike $cache_log, qr/Download error, waiting .* seconds for next try \(3 remaining\)/, 'only 3 attempts';
$cache_log = '';
};

Expand Down Expand Up @@ -112,14 +112,14 @@ subtest 'Success' => sub {

subtest 'Connection closed early' => sub {
my $from = "http://$host/tests/922756/asset/hdd/[email protected]";
like $downloader->download($from, $to), qr/Download of "$to" failed: 521 Premature connection close/, 'Failed';
like $downloader->download($from, $to), qr/Download of "$to" failed: Premature connection close/, 'Failed';

ok !-e $to, 'File not downloaded';

like $cache_log, qr/Downloading "test.qcow" from "$from"/, 'Download attempt';
like $cache_log, qr/Download of "$to" failed: 521 Premature connection close/, 'Real error is logged';
like $cache_log, qr/Download error 521, waiting .* seconds for next try \(2 remaining\)/, '2 tries remaining';
like $cache_log, qr/Download error 521, waiting .* seconds for next try \(1 remaining\)/, '1 tries remaining';
like $cache_log, qr/Download of "$to" failed: Premature connection close/, 'Real error is logged';
like $cache_log, qr/Download error, waiting .* seconds for next try \(2 remaining\)/, '2 tries remaining';
like $cache_log, qr/Download error, waiting .* seconds for next try \(1 remaining\)/, '1 tries remaining';
$cache_log = '';
};

Expand All @@ -144,8 +144,8 @@ subtest 'Size differs' => sub {

like $cache_log, qr/Downloading "test.qcow" from "$from"/, 'Download attempt';
like $cache_log, qr/Size of .+ differs, expected 10 Byte but downloaded 6 Byte/, 'Incomplete download logged';
like $cache_log, qr/Download error 598, waiting .* seconds for next try \(2 remaining\)/, '2 tries remaining';
like $cache_log, qr/Download error 598, waiting .* seconds for next try \(1 remaining\)/, '1 tries remaining';
like $cache_log, qr/Download error, waiting .* seconds for next try \(2 remaining\)/, '2 tries remaining';
like $cache_log, qr/Download error, waiting .* seconds for next try \(1 remaining\)/, '1 tries remaining';
$cache_log = '';
};

Expand Down

0 comments on commit 85500a2

Please sign in to comment.