Skip to content

Commit

Permalink
Fix hard download error on 404 etc
Browse files Browse the repository at this point in the history
I've observed errors when some mirrors are not completely synced. The
library tries to download a file, but gets a 404 error. This means we
need to retry with another mirror, not crash out. This was crashing
because setting `err` in `clean_transcode()` was firing the assert at
the start of `truncate_transfer_file()`. Note this failure mode was most
common with 404's, but any transfer error could likely have turned
fatal, for invalid reasons.

We use `cleanup_transcode()` in two contexts.

1. Within `check_transfer_statuses()`. The first call here happens
   during a normal download after `check_finished_transfer_status()`.
   The cleanup checks for errors, and any here will be flagged as a
   `transfer_err` (not a general, err).
2. In 3 other places where an error has already occurred. We need to
   wait for the program to exit (and it should stop due to a SIGPIPE
   or short read from stdin), but we don't need to set an error because
   something already is handling one. It doesn't matter that the
   transcoder crashed because we're not going to use the output anyway,
   and we are likely to retry.
  • Loading branch information
malmond77 committed Apr 21, 2021
1 parent 638dd0a commit 566400a
Showing 1 changed file with 14 additions and 10 deletions.
24 changes: 14 additions & 10 deletions librepo/downloader.c
Original file line number Diff line number Diff line change
Expand Up @@ -1530,8 +1530,12 @@ maybe_transcode(LrTarget *target, GError **err)
}

void
cleanup_transcode(LrTarget *target, GError **err)
cleanup_transcode(LrTarget *target, GError **transfer_err)
{
/** transfer_err can be NULL if we're using this to clean up a failed
* transfer. In that circumstance g_set_error does nothing which is fine,
* we don't need to pile on a second failure reason.
*/
int wstatus, trc;
if (!target->writef) {
return;
Expand All @@ -1541,21 +1545,21 @@ cleanup_transcode(LrTarget *target, GError **err)
}
fclose(target->writef);
if(waitpid(target->pid, &wstatus, 0) == -1) {
g_set_error(err, LR_DOWNLOADER_ERROR, LRE_TRANSCODE,
g_set_error(transfer_err, LR_DOWNLOADER_ERROR, LRE_TRANSCODE,
"transcode waitpid failed: %s", g_strerror(errno));
} else if (WIFEXITED(wstatus)) {
trc = WEXITSTATUS(wstatus);
if (trc != 0) {
g_set_error(err, LR_DOWNLOADER_ERROR, LRE_TRANSCODE,
g_set_error(transfer_err, LR_DOWNLOADER_ERROR, LRE_TRANSCODE,
"transcode process non-zero exit code %d", trc);
}
} else if (WIFSIGNALED(wstatus)) {
g_set_error(err, LR_DOWNLOADER_ERROR, LRE_TRANSCODE,
g_set_error(transfer_err, LR_DOWNLOADER_ERROR, LRE_TRANSCODE,
"transcode process was terminated with a signal: %d",
WTERMSIG(wstatus));
} else {
/* don't think this can happen, but covering all bases */
g_set_error(err, LR_DOWNLOADER_ERROR, LRE_TRANSCODE,
g_set_error(transfer_err, LR_DOWNLOADER_ERROR, LRE_TRANSCODE,
"transcode unhandled circumstance in waitpid");
}
target->writef = NULL;
Expand Down Expand Up @@ -1822,7 +1826,7 @@ prepare_next_transfer(LrDownload *dd, gboolean *candidatefound, GError **err)
curl_easy_cleanup(target->curl_handle);
target->curl_handle = NULL;
}
cleanup_transcode(target, err);
cleanup_transcode(target, NULL);
if (target->f != NULL) {
fclose(target->f);
target->f = NULL;
Expand Down Expand Up @@ -2390,11 +2394,11 @@ check_transfer_statuses(LrDownload *dd, GError **err)
if (!ret) // Error
return FALSE;

cleanup_transcode(target, &transfer_err);

if (transfer_err) // Transfer was unsuccessful
goto transfer_error;

cleanup_transcode(target, err);

//
// Checksum checking
//
Expand Down Expand Up @@ -2489,7 +2493,7 @@ check_transfer_statuses(LrDownload *dd, GError **err)
target->curl_handle = NULL;
g_free(target->headercb_interrupt_reason);
target->headercb_interrupt_reason = NULL;
cleanup_transcode(target, err);
cleanup_transcode(target, NULL);
fclose(target->f);
target->f = NULL;
if (target->curl_rqheaders) {
Expand Down Expand Up @@ -2893,7 +2897,7 @@ lr_download(GSList *targets,
curl_multi_remove_handle(dd.multi_handle, target->curl_handle);
curl_easy_cleanup(target->curl_handle);
target->curl_handle = NULL;
cleanup_transcode(target, err);
cleanup_transcode(target, NULL);
fclose(target->f);
target->f = NULL;
g_free(target->headercb_interrupt_reason);
Expand Down

0 comments on commit 566400a

Please sign in to comment.