Skip to content

Commit

Permalink
Merge pull request #5810 from Martchus/downloader-error-handling
Browse files Browse the repository at this point in the history 
Simplify error handling of downloader, avoid confusing error codes
  • Loading branch information
@mergify
mergify[bot] authored Aug 1, 2024
2 parents 4726870 + 0be1071 commit 64488e5
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 46 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
21 changes: 10 additions & 11 deletions t/25-cache.t
View file
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ $cache_log = '';
$cache->get_asset($host, {id => 922756}, 'hdd', '[email protected]');
my $from = "http://$host/tests/922756/asset/hdd/[email protected]";
like $cache_log, qr/Downloading "sle-12-SP3-x86_64-0368-textmode\@64bit.qcow2" from "$from"/, 'Asset download attempt';
like $cache_log, qr/failed: 521/, 'Asset download fails with: 521 Connection refused';
like $cache_log, qr/failed: Connection refused/, 'Asset download fails with: Connection refused';
$cache_log = '';

$port = Mojo::IOLoop::Server->generate_port;
Expand Down Expand Up @@ -195,23 +195,22 @@ $cache->get_asset($host, {id => 922756}, 'hdd', 'sle-12-SP3-x86_64-0368-589@64bi
$cache->downloader->ua->inactivity_timeout($old_timeout);
like $cache_log, qr/Downloading "sle-12-SP3-x86_64-0368-589\@64bit.qcow2" from/, 'Asset 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 0.01 seconds for next try \(4 remaining\)/, '4 tries remaining';
like $cache_log, qr/Download error 598, waiting 0.01 seconds for next try \(3 remaining\)/, '3 tries remaining';
like $cache_log, qr/Download error 598, waiting 0.01 seconds for next try \(2 remaining\)/, '2 tries remaining';
like $cache_log, qr/Download error 598, waiting 0.01 seconds for next try \(1 remaining\)/, '1 tries remaining';
like $cache_log, qr/Download error, waiting 0.01 seconds for next try \(4 remaining\)/, '4 tries remaining';
like $cache_log, qr/Download error, waiting 0.01 seconds for next try \(3 remaining\)/, '3 tries remaining';
like $cache_log, qr/Download error, waiting 0.01 seconds for next try \(2 remaining\)/, '2 tries remaining';
like $cache_log, qr/Download error, waiting 0.01 seconds for next try \(1 remaining\)/, '1 tries remaining';
like $cache_log, qr/Purging ".*qcow2" because of too many download errors/, 'Bailing out after too many retries';
ok !-e $cachedir->child($host, '[email protected]'), 'Asset does not exist in cache';
$cache_log = '';

# Retry connection error (closed early)
$cache->get_asset($host, {id => 922756}, 'hdd', '[email protected]');
like $cache_log, qr/Downloading "sle-12-SP3-x86_64-0368-200_close\@64bit.qcow2" from/, 'Asset download attempt';
like $cache_log, qr/Download of ".*200_close\@64bit.qcow2" failed: 521 Premature connection close/,
'Real error is logged';
like $cache_log, qr/Download error 521, waiting 0.01 seconds for next try \(4 remaining\)/, '4 tries remaining';
like $cache_log, qr/Download error 521, waiting 0.01 seconds for next try \(3 remaining\)/, '3 tries remaining';
like $cache_log, qr/Download error 521, waiting 0.01 seconds for next try \(2 remaining\)/, '2 tries remaining';
like $cache_log, qr/Download error 521, waiting 0.01 seconds for next try \(1 remaining\)/, '1 tries remaining';
like $cache_log, qr/Download of ".*200_close\@64bit.qcow2" failed: Premature connection close/, 'Real error is logged';
like $cache_log, qr/Download error, waiting 0.01 seconds for next try \(4 remaining\)/, '4 tries remaining';
like $cache_log, qr/Download error, waiting 0.01 seconds for next try \(3 remaining\)/, '3 tries remaining';
like $cache_log, qr/Download error, waiting 0.01 seconds for next try \(2 remaining\)/, '2 tries remaining';
like $cache_log, qr/Download error, waiting 0.01 seconds for next try \(1 remaining\)/, '1 tries remaining';
like $cache_log, qr/Purging ".*200_close\@64bit.qcow2" because of too many download errors/,
'Bailing out after too many retries';
like $cache_log, qr/Purging ".*200_close\@64bit.qcow2" failed because the asset did not exist/, 'Asset was missing';
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 64488e5

Please sign in to comment.