refactor: clean up some panics in favor of returning a Result #5967

marcelo-gonzalez · 2021-12-23T21:34:13Z

Here we change things so that functions that might panic on bad user input return a result instead, so we can exit normally from the init command. This doesn't get rid of every case, but tries to chip away at it a little.

Issue: #5485

Also make the error message whehn --genesis and --download-genesis are both given a little more clear.

pmnoxx · 2021-12-24T06:42:24Z

core/crypto/src/key_file.rs

        #[cfg(platform = "unix")]
        {
            use std::os::unix::fs::PermissionsExt;
            let perm = std::fs::Permissions::from_mode(u32::from(libc::S_IWUSR | libc::S_IRUSR));
-            file.set_permissions(perm).expect("Failed to set permissions for a key file.");
-        }
-        let str = serde_json::to_string_pretty(self).expect("Error serializing the key file.");


It may be a good idea to print the path, where this operation failed. That, would make it easier to debug if something happens. Otherwise, you kind of have to modify the code, to figure out what failed.

Ah yea so what I tried to do here was make the callers print out the path in config.rs, like:

network_signer .write_to_file(&path) .with_context(|| format!("Error writing key file to {}", path.display()))?

But now that I look at it again maybe it's easier to do that in this function here, yeah. Since we'd have to do that in every caller anyway. Maybe the way to go is to make this function return an anyhow::Result<()> and call with_context in here?

pmnoxx

LGTM

…oes wrong This was missed, but near#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

…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

near#5967 refactored things so that the init command wouldn't panic on normal/expected errors, but it inadvertenly changed things so that we always get exit code 0 even when the command fails, which is quite sad. So just return an `anyhow::Error` from `InitCmd::run()`

#5967 refactored things so that the init command wouldn't panic on normal/expected errors, but it inadvertenly changed things so that we always get exit code 0 even when the command fails, which is quite sad. So just return an `anyhow::Error` from `InitCmd::run()`

near#5967 refactored things so that the init command wouldn't panic on normal/expected errors, but it inadvertenly changed things so that we always get exit code 0 even when the command fails, which is quite sad. So just return an `anyhow::Error` from `InitCmd::run()`

marcelo-gonzalez added 4 commits December 23, 2021 16:30

Return a Result from download_{config,genesis}()

aa14bd2

Return a Result from Config::write_to_file

277241c

Return a Result from the signer write_to_file() functions

11d0822

Log init_configs() errors and exit normally rather than panicking

ff5825a

Also make the error message whehn --genesis and --download-genesis are both given a little more clear.

marcelo-gonzalez requested review from bowenwang1996, matklad, mm-near and nikurt as code owners December 23, 2021 21:34

marcelo-gonzalez requested a review from pmnoxx December 23, 2021 21:34

pmnoxx reviewed Dec 24, 2021

View reviewed changes

bowenwang1996 approved these changes Dec 28, 2021

View reviewed changes

bowenwang1996 requested a review from nagisa December 28, 2021 19:10

pmnoxx approved these changes Dec 28, 2021

View reviewed changes

marcelo-gonzalez added 2 commits December 29, 2021 10:35

Merge branch 'refs/heads/master' into panic-cleanups

5bb36cc

Merge branch 'refs/heads/master' into panic-cleanups

6dd44cf

nikurt approved these changes Jan 6, 2022

View reviewed changes

Merge branch 'master' into panic-cleanups

4a9c342

marcelo-gonzalez merged commit f39a30d into near:master Jan 6, 2022

marcelo-gonzalez deleted the panic-cleanups branch January 6, 2022 14:18

marcelo-gonzalez mentioned this pull request Jan 7, 2022

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

Merged

marcelo-gonzalez mentioned this pull request Jan 11, 2023

fix: exit with nonzero code on init cmd error #8334

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: clean up some panics in favor of returning a Result #5967

refactor: clean up some panics in favor of returning a Result #5967

marcelo-gonzalez commented Dec 23, 2021

pmnoxx Dec 24, 2021

marcelo-gonzalez Dec 24, 2021

pmnoxx left a comment

refactor: clean up some panics in favor of returning a Result #5967

refactor: clean up some panics in favor of returning a Result #5967

Conversation

marcelo-gonzalez commented Dec 23, 2021

pmnoxx Dec 24, 2021

Choose a reason for hiding this comment

marcelo-gonzalez Dec 24, 2021

Choose a reason for hiding this comment

pmnoxx left a comment

Choose a reason for hiding this comment