From 6cc2fdcefb2a2d6a5ac4a2299be125d41c94bca1 Mon Sep 17 00:00:00 2001 From: Marcelo Diop-Gonzalez Date: Fri, 7 Jan 2022 12:19:11 -0500 Subject: [PATCH 1/2] fix(error messages): fix error messages when downloading the config goes wrong This was missed, but https://github.com/near/nearcore/pull/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 --- nearcore/src/config.rs | 84 +++++++++++++++++++++++------------------- 1 file changed, 46 insertions(+), 38 deletions(-) diff --git a/nearcore/src/config.rs b/nearcore/src/config.rs index 49d49bef887..d14e50cd351 100644 --- a/nearcore/src/config.rs +++ b/nearcore/src/config.rs @@ -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))?; } @@ -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, @@ -1137,17 +1137,17 @@ 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}")] + 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), } @@ -1155,14 +1155,17 @@ pub enum FileDownloadError { /// `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))?; } Ok(()) } @@ -1170,32 +1173,37 @@ async fn download_file_impl( /// 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> { From 86a3654f12dfd94548f872dec45c824726910011 Mon Sep 17 00:00:00 2001 From: Marcelo Diop-Gonzalez Date: Fri, 7 Jan 2022 13:52:49 -0500 Subject: [PATCH 2/2] use the #[source] attribute instead of directly formatting errors and store a PathBuf instead of a String for paths in error variants --- nearcore/src/config.rs | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/nearcore/src/config.rs b/nearcore/src/config.rs index d14e50cd351..f0dc81c74b4 100644 --- a/nearcore/src/config.rs +++ b/nearcore/src/config.rs @@ -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; @@ -1139,16 +1139,16 @@ pub fn get_config_url(chain_id: &str) -> String { pub enum FileDownloadError { #[error("{0}")] HttpError(hyper::Error), - #[error("Failed to open temporary file: {0}")] - OpenError(std::io::Error), - #[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("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 temporary file: {0}. Download previously failed: {1}")] - RemoveTemporaryFileError(std::io::Error, Box), + #[error("Failed to remove temporary file: {0}. Download previously failed")] + RemoveTemporaryFileError(std::io::Error, #[source] Box), } /// Downloads resource at given `uri` and saves it to `file`. On failure, @@ -1165,7 +1165,7 @@ async fn download_file_impl( 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))?; + .map_err(|e| FileDownloadError::WriteError(path.to_path_buf(), e))?; } Ok(()) } @@ -1190,8 +1190,8 @@ pub fn download_file(url: &str, path: &Path) -> Result<(), FileDownloadError> { { 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(); + let from = e.path.to_path_buf(); + let to = path.to_path_buf(); (e.path, FileDownloadError::RenameError(from, to, e.error)) }), };