Skip to content

Commit

Permalink
fix(error messages): fix error messages when downloading the config (#…
Browse files Browse the repository at this point in the history
…6024)

This was missed, but #5967 did
not interact very well with `enum FileDownloadError`, so this is an
attempt to fix that, and present better error messages.

The reason for getting rid of the #[from] in the HttpError variant is
that it doesn't really play nicely with the anyhow {:#} formatter and
gives us ugliness like:

ERROR neard::cli: Failed to initialize configs: Failed to download the config file from http://localhost:8002/config.json: error trying to connect: tcp connect error: Connection refused (os error 111): tcp connect error: Connection refused (os error 111): Connection refused (os error 111)

Test Plan:
Of course check that downloading the file still works, but also inject
errors and make sure the error messages look ok. For reviewer
convenience, error msgs corresponding to the variants below (except
for RemoveTemporaryFileError. Didn't get to that one... but I think
it's probably good)

HttpError:

ERROR neard::cli: Failed to initialize configs: Failed to download the config file from http://localhost:8002/config.json: error trying to connect: tcp connect error: Connection refused (os error 111)

OpenError:

ERROR neard::cli: Failed to initialize configs: Failed to download the config file from http://localhost:8002/config.json: Failed to open temporary file: No such file or directory (os error 2) at path "/asdf/.tmp2g9Pqe"

WriteError:

ERROR neard::cli: Failed to initialize configs: Failed to download the config file from http://localhost:8000/config.json: Failed to write to temporary file at /tmp/asdfa/.tmpNdy4iL: Bad file descriptor (os error 9)

RenameEror:

ERROR neard::cli: Failed to initialize configs: Failed to download the config file from http://localhost:8000/config.json: Failed to rename temporary file /tmp/asdfa/.tmpRvBBEn to /tmp/asdfa/config.json : Permission denied (os error 13)

UriError:

ERROR neard::cli: Failed to initialize configs: Failed to download the config file from http://localhost:8002:3:a:b/config.json: Invalid URI: invalid authority

* use the #[source] attribute instead of directly formatting errors
and store a PathBuf instead of a String for paths in error variants
  • Loading branch information
marcelo-gonzalez authored Jan 7, 2022
1 parent c7ef817 commit 41fc061
Showing 1 changed file with 49 additions and 41 deletions.
90 changes: 49 additions & 41 deletions nearcore/src/config.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::fs;
use std::fs::File;
use std::io::{Read, Write};
use std::path::Path;
use std::path::{Path, PathBuf};
use std::sync::Arc;
use std::time::Duration;

Expand Down Expand Up @@ -843,12 +843,12 @@ pub fn init_configs(

if let Some(url) = download_config_url {
download_config(&url.to_string(), &dir.join(CONFIG_FILENAME))
.context("Failed to download the config file")?;
.context(format!("Failed to download the config file from {}", url))?;
config = Config::from_file(&dir.join(CONFIG_FILENAME))?;
} else if should_download_config {
let url = get_config_url(&chain_id);
download_config(&url, &dir.join(CONFIG_FILENAME))
.context("Failed to download the config file")?;
.context(format!("Failed to download the config file from {}", url))?;
config = Config::from_file(&dir.join(CONFIG_FILENAME))?;
}

Expand Down Expand Up @@ -910,11 +910,11 @@ pub fn init_configs(

if let Some(url) = download_genesis_url {
download_genesis(&url.to_string(), &genesis_path)
.context("Failed to download the genesis file")?;
.context(format!("Failed to download the genesis file from {}", url))?;
} else if should_download_genesis {
let url = get_genesis_url(&chain_id);
download_genesis(&url, &genesis_path)
.context("Failed to download the genesis file")?;
.context(format!("Failed to download the genesis file from {}", url))?;
} else {
genesis_path_str = match genesis {
Some(g) => g,
Expand Down Expand Up @@ -1137,65 +1137,73 @@ pub fn get_config_url(chain_id: &str) -> String {

#[derive(thiserror::Error, Debug)]
pub enum FileDownloadError {
#[error("Failed to download the file: {0}")]
HttpError(#[from] hyper::Error),
#[error("Failed to open file: {0}")]
OpenError(std::io::Error),
#[error("Failed to write to file: {0}")]
WriteError(std::io::Error),
#[error("Failed to rename file: {0}")]
RenameError(std::io::Error),
#[error("Invalid URI: {0}")]
#[error("{0}")]
HttpError(hyper::Error),
#[error("Failed to open temporary file")]
OpenError(#[source] std::io::Error),
#[error("Failed to write to temporary file at {0:?}")]
WriteError(PathBuf, #[source] std::io::Error),
#[error("Failed to rename temporary file {0:?} to {1:?}")]
RenameError(PathBuf, PathBuf, #[source] std::io::Error),
#[error("Invalid URI")]
UriError(#[from] hyper::http::uri::InvalidUri),
#[error("Failed to remove the temporary file after failure: {0}, {1}")]
RemoveTemporaryFileError(std::io::Error, Box<FileDownloadError>),
#[error("Failed to remove temporary file: {0}. Download previously failed")]
RemoveTemporaryFileError(std::io::Error, #[source] Box<FileDownloadError>),
}

/// Downloads resource at given `uri` and saves it to `file`. On failure,
/// `file` may be left in inconsistent state (i.e. may contain partial data).
async fn download_file_impl(
uri: hyper::Uri,
path: &tempfile::TempPath,
mut file: tokio::fs::File,
) -> anyhow::Result<(), FileDownloadError> {
let https_connector = hyper_tls::HttpsConnector::new();
let client = hyper::Client::builder().build::<_, hyper::Body>(https_connector);
let mut resp = client.get(uri).await?;
let mut resp = client.get(uri).await.map_err(FileDownloadError::HttpError)?;
while let Some(next_chunk_result) = resp.data().await {
let next_chunk = next_chunk_result?;
file.write_all(next_chunk.as_ref()).await.map_err(FileDownloadError::WriteError)?;
let next_chunk = next_chunk_result.map_err(FileDownloadError::HttpError)?;
file.write_all(next_chunk.as_ref())
.await
.map_err(|e| FileDownloadError::WriteError(path.to_path_buf(), e))?;
}
Ok(())
}

/// Downloads a resource at given `url` and saves it to `path`. On success, if
/// file at `path` exists it will be overwritten. On failure, file at `path` is
/// left unchanged (if it exists).
pub fn download_file(url: &str, path: &Path) -> anyhow::Result<(), FileDownloadError> {
pub fn download_file(url: &str, path: &Path) -> Result<(), FileDownloadError> {
let uri = url.parse()?;
let (tmp_file, tmp_path) = {
let tmp_dir = path.parent().unwrap_or(Path::new("."));
tempfile::NamedTempFile::new_in(tmp_dir).map_err(FileDownloadError::OpenError)?.into_parts()
};

let result =
tokio::runtime::Builder::new_current_thread().enable_all().build().unwrap().block_on(
async move {
let tmp_file = tokio::fs::File::from_std(tmp_file);
download_file_impl(uri, tmp_file).await
},
);
tokio::runtime::Builder::new_current_thread().enable_all().build().unwrap().block_on(
async move {
let (tmp_file, tmp_path) = {
let tmp_dir = path.parent().unwrap_or(Path::new("."));
tempfile::NamedTempFile::new_in(tmp_dir)
.map_err(FileDownloadError::OpenError)?
.into_parts()
};

let result = match result {
Err(err) => Err((tmp_path, err)),
Ok(()) => {
tmp_path.persist(path).map_err(|e| (e.path, FileDownloadError::RenameError(e.error)))
}
};
let result =
match download_file_impl(uri, &tmp_path, tokio::fs::File::from_std(tmp_file)).await
{
Err(err) => Err((tmp_path, err)),
Ok(()) => tmp_path.persist(path).map_err(|e| {
let from = e.path.to_path_buf();
let to = path.to_path_buf();
(e.path, FileDownloadError::RenameError(from, to, e.error))
}),
};

result.map_err(|(tmp_path, err)| match tmp_path.close() {
Ok(()) => err,
Err(close_err) => FileDownloadError::RemoveTemporaryFileError(close_err, Box::new(err)),
})
result.map_err(|(tmp_path, err)| match tmp_path.close() {
Ok(()) => err,
Err(close_err) => {
FileDownloadError::RemoveTemporaryFileError(close_err, Box::new(err))
}
})
},
)
}

pub fn download_genesis(url: &str, path: &Path) -> Result<(), FileDownloadError> {
Expand Down

0 comments on commit 41fc061

Please sign in to comment.