Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(error messages): fix error messages when downloading the config #6024

Merged
merged 4 commits into from
Jan 7, 2022
Merged

fix(error messages): fix error messages when downloading the config #6024

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6cc2fdc
fix(error messages): fix error messages when downloading the config g…
marcelo-gonzalez Jan 7, 2022
86a3654
use the #[source] attribute instead of directly formatting errors
marcelo-gonzalez Jan 7, 2022
a86ef39
Merge branch 'master' into error-msgs
marcelo-gonzalez Jan 7, 2022
0ebb6f2
Merge branch 'master' into error-msgs
marcelo-gonzalez Jan 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 46 additions & 38 deletions nearcore/src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
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}")]
#[error("{0}")]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be #[error(transparent)] I think.

Pre-existing, but in general thiserror messages should not format in the cause error's description into its own. Instead #[source] mechanism should be utilized to specify the causation chain. This is how the ecosystem expects the errors to be, making it least likely to get duplicated output like you had described in the PR description. I have, in the past, also written a block post on error handling. While this is not directly applicable to this discussion, it kind of demonstrates what a conventional thiserror use looks like.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah, I actually tried this originally but it gives the same repition of info. Looks like something wrong with hyper itself?

println!("orig {}", &e);
let mut source = e.source();

while let Some(err) = source {
  println!("source {}", err);
  source = err.source();
}

this prints out:

orig error trying to connect: tcp connect error: Connection refused (os error 111)
source tcp connect error: Connection refused (os error 111)
source Connection refused (os error 111)

So just using "{0}" is the best workaround for that I can think of here. But as for the others, using #[source] instead of formatting it explicitly seems way better yeah, I like the blog post! updated the other ones to use #[source] now, and checked that the output is still good. (Last one is a little tricky since there are really two sources, but we can just pick the first one I guess)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I think it might be a good idea to report this to hyper.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hyperium/hyper#2732

HttpError(hyper::Error),
#[error("Failed to open temporary 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("Failed to write to temporary file at {0}: {1}")]
WriteError(String, std::io::Error),
#[error("Failed to rename temporary file {0} to {1} : {2}")]
RenameError(String, String, std::io::Error),
#[error("Invalid URI")]
UriError(#[from] hyper::http::uri::InvalidUri),
#[error("Failed to remove the temporary file after failure: {0}, {1}")]
#[error("Failed to remove temporary file: {0}. Download previously failed: {1}")]
RemoveTemporaryFileError(std::io::Error, 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_string_lossy().into_owned(), e))?;
marcelo-gonzalez marked this conversation as resolved.
Show resolved Hide resolved
}
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_string_lossy().into_owned();
let to = path.to_string_lossy().into_owned();
(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